fix: Update regex for formatting assertion messages to avoid catastrophic backtracking #29353
Conversation
This file contains 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
…ly before regex runs
jennifer-shehane
changed the title
AtofStryker
requested changes
Apr 18, 2024
|
@AtofStryker Yah, I'm trying to do that. It's kind of hard to find a test case in the goldilocks zone that fixes it but doesn't make a really long test. I'm about to try to replicate exactly what the plugin does as a test. |
|
Concerned that a test could be performant on my Mac, but then hang on CI. It seems very specific to a certain size of string where it starts hanging. I opened a PR to add a test at some point: #29356 |
AtofStryker
approved these changes
Apr 18, 2024
ryanthemanuel
approved these changes
Apr 18, 2024
Passing run #55011 ↗︎
Details:
Review all test suite changes for PR #29353 ↗︎ |
|||||||||||||||
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Additional details
This fixes a regression that was introduced in #29243. Those changes can result in catastrophic backtracking when trying to match long assertion messages via the new Regex.
Additionally, I did restructure some of the code so that if certain conditions are not met where the message does not need to be run through the regex, it is no longer run through the regex. This should improve performance for some commands that were needlessly going through the regex check.
Steps to test
Run this repo with 13.7.3 and run it with this branch. The test should now pass in this branch.
I tried to write a test to ensure Cypress does not hang, but my test seems unpredictable. I haven't figured out the exact size of the assertion of content that will cause it to hang before and not after (without it being a complicated test including the plugin). I intend to followup with a PR to test this and am favoring getting this out quickly.
How has the user experience changed?
Before
After
PR Tasks
cypress-documentation?type definitions?