-
Notifications
You must be signed in to change notification settings - Fork 50.4k
[tests] Require exact error messages in assertConsole helpers #35497
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
Update test assertions to include the complete component stack trace rather than partial stacks. This ensures tests validate the full owner stack as it would appear in development.
|
Comparing: 5aec1b2...e61f352 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
Add two new placeholders for console error assertions: 1. [Server] - expands to the ANSI escape sequence for server badge Instead of: '\u001b[0m\u001b[7m Server \u001b[0mError: message' Write: '[Server] Error: message' 2. \n in <stack> - matches JavaScript Error stack traces Instead of matching the full error stack manually Write: 'Error: message\n in <stack>' The error stack placeholder validates that it's only used for actual Error stack traces (messages starting with "Error:" that have file:line:col frames), not for React component stacks. Also adds validation to catch misuse of these placeholders and provides helpful error messages guiding developers to the correct usage.
1a33282 to
eb04af7
Compare
eps1lon
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.
Love it!
| // Error stack traces start with "Error:" and contain "at" frames with file paths | ||
| // Component stacks contain "in ComponentName" patterns | ||
| // This helps validate that \n in <stack> is used correctly | ||
| const isLikelyAnErrorStackTrace = message => | ||
| typeof message === 'string' && | ||
| message.includes('Error:') && |
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.
Your implementation already accounts for e.g. TypeError() having TypeError: bla when stringified but the comments still says "start". Should prob update the comment to call out that includes is used to handle other kind of errors.
Also the input here is the stringified error instance not just the message. .message doesn't include the error name.
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.
Good call, I noticed this while vibe coding too and meant to get back to 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.
Updated the comment - The message does include the error name (and stack trace). I was surprised about this too.
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 updated the tests to log an error, showing the behavior (and confirmed with logging).
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 the comment - The message does include the error name (and stack trace). I was surprised about this too.
That sounds wrong. Maybe you're reading .stack instead of .message? .stack would include name, message, and stacktrace.
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.
If it was wrong, then this wouldn't work right? Like this function would noop, but if you noop it then the tests fail. Maybe there's a shim or console override adding it?
Requires full error message in assert helpers.
Some of the error messages we asset on add a native javascript stack trace, which would be a pain to add to the messages and maintain. This PR allows you to just add
\n in <stack>placeholder to the error message to denote a native stack trace is present in the message.Note: i vibe coded this so it was a pain to backtrack this to break this into a stack, I tried and gave up, sorry.