-
Notifications
You must be signed in to change notification settings - Fork 5k
feat: proper source maps for spec.md #38773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| handleUncaughtExceptions: false, | ||
| retrieveSourceMap(source) { | ||
| if (source.startsWith('file://')) | ||
| source = source.substring('file://'.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for me things worked without this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow, did not work for me. Perhaps it depends on how the loader worked though...
| return `\n test(${escapeString(test.title)}, ${props}async ({ page, agent }) => {\n` + | ||
| test.lines.map(line => ' ' + line).join('\n') + `\n });\n`; | ||
| }); | ||
| // TODO: proper source mapping for props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do it straight away, otherwise tests end up attributed to the wrong file. it's pretty straight-forward, make the annotations multi-line arrays and give each prop its own line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see you dropped the newlines, so it won't break anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to follow up 😄 This does not affect the test location.
| const result = `${importLine}\ntest.describe(${escapeString(parsed.describe)}, () => {${renderedTests.join('')}\n});\n`; | ||
| return result; | ||
| const encodedMap = genMapping.toEncodedMap(map); | ||
| const result = lines.join('\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we add an inline sourcemap through babel, but not here. let's add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We return the map to be consumed by the caller, why would we add a inline one? I don't really understand what to write in the comment.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
307ed22 to
650d241
Compare
Test results for "tests 1"7 failed 6 flaky34444 passed, 696 skipped Merge workflow run. |
Test results for "MCP"2822 passed, 116 skipped Merge workflow run. |
Based on #38760.