-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix(reporters): error context exception with page.evaluate #35670
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
This comment has been minimized.
This comment has been minimized.
Skn0tt
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'm not convinced this is the right fix. Could we walk parsedError to find the first error location that we can read instead?
de5bfdf to
1555606
Compare
This comment has been minimized.
This comment has been minimized.
Skn0tt
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 like it! cc @dgozman for final review.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
after upgrade 1 of our tests started failing blocking the upgrade should I expect it to be fixed with this PR |
Looks different, so a new bug would be appreciated! |
| source, | ||
| { | ||
| start: { | ||
| line: location.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.
I think we should synchronize location and source variables - try loading source from the parsedError.location first, and then fallback to the test location and source.
This comment has been minimized.
This comment has been minimized.
7bebbd5 to
b18035f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"4 flaky39063 passed, 799 skipped Merge workflow run. |
Minimal repro:
Stack:
We can fix it separately I think. We should re-visit our idea of making
page.evaluateerrors anerror.cause+ make this non-throwing.