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

Add a unit test from aspnetcore #56108

Merged
merged 5 commits into from
Jul 29, 2021
Merged

Conversation

pgovind
Copy link
Contributor

@pgovind pgovind commented Jul 21, 2021

Add a unit test that covers a critical scenario from ASP.NET. This scenario was identified when #51508 was merged in and caused a regression in ASP.NET, as reported when the change was backported to preview 7. That regression was first observed in dotnet/aspnetcore#34491.

Rule: (.*)/(.*).aspx
Input: /pages/homepage.aspx

Expected backreferences:

  1. /pages/homepage.aspx
  2. /pages
  3. homepage

Actual backreferences: None, didn't match

This scenario represents multiple .* groups within a pattern and being able to capture all expected backreferences.

@ghost
Copy link

ghost commented Jul 21, 2021

Tagging subscribers to this area: @eerhardt, @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

Just adding a unit test to our tests.

Author: pgovind
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

@jeffhandley jeffhandley self-assigned this Jul 24, 2021
@pgovind
Copy link
Contributor Author

pgovind commented Jul 29, 2021

GH won't let me approve this PR since I opened it. The changes look good to me. There are interesting patterns to add with .? too, but I'll do that on my next pass through these files.

@ghost
Copy link

ghost commented Jul 29, 2021

Hello @jeffhandley!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@jeffhandley
Copy link
Member

Thanks, @pgovind. Good thinking on .?, but per your comment, I won't add them here/now.

I noticed that there's very little coverage of negative scenarios where matches are unsuccessful. One of the mistakes I made yesterday was assuming I could cover such scenarios within the group tests, but that's not in place--the test method assumes every scenario is a successful match. There are a few other tests in this feature area for unsuccessful matches, but nowhere near the coverage we have in these group tests. I held back from updating this particular test suite to capture such scenarios.

@jeffhandley
Copy link
Member

Failure is unrelated and infrastructural.

@jeffhandley jeffhandley merged commit 668a39e into dotnet:main Jul 29, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants