-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: normalize test body invocationDetails from stack traces
#32699
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
base: develop
Are you sure you want to change the base?
Conversation
… invocationDetails
cypress
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
astone123/fix-itgrep-trace
|
| Run status |
|
| Run duration | 18m 53s |
| Commit |
|
| Committer | astone123 |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
1
|
|
|
12
|
|
|
1098
|
|
|
4
|
|
|
26596
|
| View all changes introduced in this branch ↗︎ | |
Warning
Partial Report: The results for the Application Quality reports may be incomplete.
UI Coverage
45.48%
|
|
|---|---|
|
|
188
|
|
|
161
|
Accessibility
97.99%
|
|
|---|---|
|
|
4 critical
8 serious
2 moderate
2 minor
|
|
|
101
|
Tests for review
cypress/e2e/commands/files.cy.js • 1 failed test • 5x-driver-firefox
|
@mschile @mabela416 |
mabela416
left 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.
I tested it locally and it works
| } | ||
| } | ||
|
|
||
| // if the stack includes the 'itGrep' function, remove any lines that include it |
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.
Likely good to reference what itGrep is and where it is coming from (same with above) https://github.com/cypress-io/cypress/blob/develop/npm/grep/src/register.ts#L77. people who aren't familiar with @cypress/grep will struggle to understand what this means.
The other concern I have is that there could be something legitimate in the stack that has itGrep in it that we want to capture but are omitting it here. Or is this only considered when trying to calculate the row/column in the spec file where the error occurred?
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.
Added a comment there. I'm pretty sure this function is specifically used to identify where the test was executed from, so this shouldn't affect anything else. If it does, it'll be limited to use of the grep plugin
|
@mschile I updated this so that it works for tests with .only as well as tests that are nested in suite definitions |
itGrep lines from stack trace when determining invocationDetailsinvocationDetails from stack traces
| ) { | ||
| lines.shift() | ||
| } | ||
| } else if (specWindow.Cypress.isBrowser('firefox')) { |
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.
Where is firefox tested?
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 need to make sure we have sufficient testing where if the stack implementation changed, our tests would fail for both chromium and firefox.
| if (specWindow.Cypress.isBrowser('chrome')) { | ||
| // There are cases where there are other lines in the stack trace before the invocation (eg. `context.it.only`, `createRunnable`, etc) | ||
| // Remove lines from the start until the top line starts with 'at eval' or 'at Suite.eval' so that we only keep the actual invocation line. | ||
| while ( |
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.
Suggest using _.dropWhile (also used elsewhere in the file).
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.
updated
| return splitAtAt.length > 1 && splitAtAt[0].trim().length === 0 | ||
| } | ||
|
|
||
| while ( |
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.
Same here.
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.
updated
| }) | ||
|
|
||
| // if we removed all the lines then something went wrong. return the original stack instead | ||
| if (modifiedStack.length === 0) { |
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 don't think this works since the error will still be on the stack as shown with the return value in stackWithLinesRemoved.
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.
updated this logic to return the original stack if the modified stack is Error
| }) | ||
|
|
||
| it('returns the correct invocation details for a grep stack trace for a test body', () => { | ||
| const stack = `Error at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14) |
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.
The Error should be on it's own line:
| const stack = `Error at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14) | |
| const stack = `Error | |
| at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14) |
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.
updated
| }) | ||
|
|
||
| it('returns the original stack if it cannot be normalized for a test body', () => { | ||
| const stack = `Error at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14) |
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.
The Error should be on it's own line:
| const stack = `Error at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14) | |
| const stack = `Error | |
| at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14) |
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.
updated
| })) | ||
| }) | ||
|
|
||
| it('returns the original stack if it cannot be normalized for a test body', () => { |
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.
| it('returns the original stack if it cannot be normalized for a test body', () => { | |
| it('returns the original stack if it cannot be normalized for a test', () => { |
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.
updated
| }) | ||
| } | ||
|
|
||
| it('returns the correct invocation details for a grep stack trace', () => { |
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.
| it('returns the correct invocation details for a grep stack trace', () => { | |
| it('returns the correct invocation details for a test with a stack that needs to be trimmed', () => { |
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.
updated
| const stack = `Error\n at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14)\n | ||
| at eval (http://localhost:3000/__cypress/tests?p=cypress/e2e/spec.cy.js:14:1)\n | ||
| at eval (http://localhost:3000/__cypress/tests?p=cypress/e2e/spec.cy.js:18:12)\n | ||
| at eval (<anonymous>)\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.
| const stack = `Error\n at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14)\n | |
| at eval (http://localhost:3000/__cypress/tests?p=cypress/e2e/spec.cy.js:14:1)\n | |
| at eval (http://localhost:3000/__cypress/tests?p=cypress/e2e/spec.cy.js:18:12)\n | |
| at eval (<anonymous>)\n | |
| const stack = `Error | |
| at itGrep (http://localhost:3000/__cypress/tests?p=cypress/support/e2e.js:444:14) | |
| at eval (http://localhost:3000/__cypress/tests?p=cypress/e2e/spec.cy.js:14:1) | |
| at eval (http://localhost:3000/__cypress/tests?p=cypress/e2e/spec.cy.js:18:12) | |
| at eval (<anonymous>) |
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.
updated
| }) | ||
|
|
||
| cy.get('.hook-open-in-ide').first().invoke('show').click() | ||
| it('sends the correct invocation details for .only test body', () => { |
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.
Is this supposed to test the new logic because I don't think it is.
mschile
left 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.
Added some comments.
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
This PR closes https://github.com/cypress-io/cypress-services/issues/11991
Additional details
There are a few cases where we don't correctly derive the test body invocation details from the stack trace. This PR fixes those scenarios by looking for
at evalorat Suite.evalin Chrome stack traces, and lines with anonymous functions in Firefox stack traces. This will allow Cypress Studio and "Open in IDE" features to work in more cases than they did before.One of these situations is when
ithas been re-written asitGrepwhich happens in the@cypress/grepand@bahmutov/cy-grepplugins. This allows us to determine the correct invocation details for the test.Steps to test
I tested this with the
@bahmutov/cy-greppackage. Install the package and register it in your support file in accordance with the documentation. Open Cypress Studio by clicking the "edit in studio" button next to a test. Verify that the test content is loaded correctly and you can edit and save the test as expected.How has the user experience changed?
Users of
@bahmutov/cy-greppackage can now use Cypress studio as intendedPR Tasks
cypress-documentation?type definitions?Note
Normalizes test body invocation details from stack traces (Chrome/Firefox), updates Mocha to request test-specific details, and adds targeted tests plus changelog entry.
stack_utils: add browser-aware trimming viastackTrimmedToTestInvocation()and extendgetInvocationDetails(specWindow, root, 'test')to normalize test invocation frames.mocha.ts: pass'test'togetInvocationDetailswhen adding tests so IDE/Studio get accurate file/line/column.reporter.hooks.cy.tswith assertions for correct "Open in IDE" invocation details (before hook, basic body,.only, wrappedit).stack_utils.spec.ts.hooks/wrapped-it.cy.js.cli/CHANGELOG.mdwith a bugfix note about correcting invocation details from stack traces.Written by Cursor Bugbot for commit 4a53ed1. This will update automatically on new commits. Configure here.