fix(typescript): map included .ts files in step output - #5685
Open
luantaraschi wants to merge 1 commit into
Open
fix(typescript): map included .ts files in step output#5685luantaraschi wants to merge 1 commit into
luantaraschi wants to merge 1 commit into
Conversation
Two adjacent blocks in container.js merge a transpile mapping after compiling TypeScript. The helper block merges into store.tsFileMapping; the include/support block merges only into container.tsFileMapping. Step.line() reads store.tsFileMapping, so a step whose stack frame points into an included page object had no entry to match and was printed with the deleted .temp.mjs sibling instead of the .ts source. Error stacks were unaffected because fixErrorStack() is handed the mapping directly. The include block now merges into store as well, mirroring the helper block two hundred lines above it. Closes codeceptjs#5675
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation/Description of the PR
Resolves #5675.
@djyarber's observation that test files map correctly but included page objects do not comes down to two adjacent blocks in
lib/container.jsthat do almost the same thing and disagree about where the result goes.The helper block, around line 470, merges the transpile mapping into
store.tsFileMapping. The include/support block, around line 889, merges the same shape of mapping intocontainer.tsFileMappingonly.Step.line()inlib/step/base.js:156readsstore.tsFileMapping. So a step whose stack frame points into an included.tsmodule has no entry to match and keeps the.temp.mjspath, which by then has been deleted, hence the paths in the report.Error stacks were never affected, which is why the migration guide's promise holds for failures:
fixErrorStack()is handed the mapping object directly by the caller rather than reading it fromstore.The include block now merges into
storeas well, mirroring the helper block.Type of change
Checklist:
npm run docs) — N/A, no public API changenpm run lint)npm test)I want to be straight about the missing test. Exercising this needs a container built from a config with a TypeScript
include, transpiled for real, and I did not get a harness for that working that I would trust.test/unit/utils/typescript_test.jsdrivestranspileTypeScriptdirectly and never touches the container. A test assertingStep.line()maps a path whenstore.tsFileMappingalready holds the entry would pass before and after this change, so it would prove nothing.What I did verify is the asymmetry itself: both blocks receive the same
mappingfromtranspileTypeScript, only one writes tostore, andstoreis what the reader uses. If you point me at the right fixture or harness for a config-level include I will add the regression test.Unit suite on Windows: 758 passing / 11 failing, unchanged by this PR. Those 11 are pre-existing path assertions that expect POSIX paths and see a
C:drive letter (utils_test.js,utils/trace_test.js).