Be more general in stripping off stack frames to fix Firefox tests#2425
Merged
fatso83 merged 2 commits intosinonjs:masterfrom Jan 25, 2022
Merged
Be more general in stripping off stack frames to fix Firefox tests#2425fatso83 merged 2 commits intosinonjs:masterfrom
fatso83 merged 2 commits intosinonjs:masterfrom
Conversation
This regex was made more specific in an attempt to avoid incorrectly stripping off parts of messages - we strip off anything after " at", which means "to have been called at least once" becomes "to have been called". However, the new regex was too specific and made faulty assumptions about stack traces - namely, that they'll always have parentheses after the at. Firefox's stack traces, however, look like so: func() at callFn@about:blank line 2 > injectedScript:47353:21 So, we could try to come up with a better regex that matches all browsers that we test with, or perhaps add some sort of indicator when we're appending the stack traces, but since this change is breaking the tests for everyone right now, for expedience we'll just revert to how we were stripping things before. This means changing one of the asserts back to how it was before, where it was asserting against a partial message due to the over-eager stripping, so add a comment about that as well.
Contributor
Author
|
Okay, I was able to run the tests under Firefox now, verified that I got the failures in master, and that the tests are passing again in this branch. |
This should match any stack frame format that has at least one non-word, non-space character in it, which should be a pretty safe assumption, and holds for the three browsers we test in anyway. This gets us back to not trimming messages improperly, while also not breaking firefox, and I think is reasonable as far as regexes go. This seems like a reasonable enough regex - the original fixed one was overly complicated because it tried to remove an arbitrary number of stack frames, which was unnecessary because we only append one.
Contributor
Author
|
Okay, and now that I can test, and after thinking on it a bit, I just pushed a follow-up commit with a more general regex whose only assumption is that a stack frame has a non-word, non-space character somewhere - which seems a much safer assumption to me, and certainly holds for all the browsers we test in. This gets us back to not chopping off the ends of legitimate messages, and seems like a reasonable regex to me as far as regexes go, but feel free to revert to the previous commit if we'd rather go with the simpler fix. |
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
2 participants
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.
Purpose (TL;DR)
This fixes a breaking change to tests in Firefox introduced in #2410, by reverting a small subset of that PR.
Background (Problem in detail)
We're stripping off stack traces for comparison's sake in tests, and previously to #2410 we used a pretty non-specific regex to do so, which accidentally stripped off part of the assertion message "expected func() to have been called at least once", and thus were asserting that such an assertion results in the message "expected func() to have been called". This is technically incorrect, but also fairly harmless. Amusingly, the message itself is still semantically sound, just less specific than the original.
As part of a larger refactor in #2410, I attempted to make this regex more specific, and thus not incorrectly strip part of the assertion message along with the stack traces. This was successful in not stripping the message, but went too far in the other direction - the new regex is now too specific, and makes assumptions about stack traces that do not hold for Firefox's stack trace format.
Solution
It's likely possible to come up with a regex that will match both formats, while still not stripping off the ends of legitimate messages, but in the interest of expedience and simplicity, this PR simply reverts to the previous regex, and adjusts the one test affected to once again assert against the technically incorrect message.
How to verify - mandatory
Checklist for author
npm run lintpasses