Skip to content
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

feat: add assert deepMatch methods #1644

Closed
wants to merge 3 commits into from

Conversation

a2937
Copy link

@a2937 a2937 commented Oct 4, 2024

This adds and test two brand new assert methods. The methods are assert.deepMatch and assert.notDeepMatch. The purpose of these two methods are to ensure that full regex matches are caught like in the case of the regex "/fr[e3]|[e3]|[e3] money/i" and the test string "fr33 mon3y". While there are partial methods, there wasn't a way to assert there weren't.

Appropriate tests have been added.

Closes #1643.

@a2937 a2937 requested a review from a team as a code owner October 4, 2024 18:42
Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I have some comments.

The deep keyword already has a defined semantic in our API, so I don't think the naming is correct here.

I'm also not fully convinced we need this. Nothing in #1643 nor freeCodeCamp/freeCodeCamp#56471 is especially compelling to the use case here, in my opinion. Code camps having false positives is common enough and not really the intended use case for a testing library. I would expect to see more developers noting this as a problem when testing their applications. I doubt many people are using .exec() for regexps, and if - during testing - a false positive occurred I would imagine a developer would tweak the regex rather than looking for alternate APIs to do the same thing.

In order for this to land here's what I'd like to see:

  1. Strong justifiable use cases for this. Why does this need to be added? What problems does it solve for people testing their applications? Why would they reach for this over .match?
  2. An alternate name for the method. The word deep must not be used, perhaps full can be used in place. I'm also not enamoured with using the word match when it uses .exec() but that's not as important.
  3. More compelling unit tests that verify this functionality and how it differs from .match - the current tests don't really achieve this IMO.
  4. Likewise, some more detailed docs which describe the differences.

I would expect all 4 of these to be resolved before we merge this, but I also want to stress that time shouldn't be spent on 2-4 before resolving 1; as it stands this change isn't sufficiently justified, IMO, even after resolving 2-4.

@a2937 a2937 closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] - Add a full regex match method on assert
2 participants