Skip to content

Use RegexGenerator in applicable tests #66179

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

Merged
merged 35 commits into from
Mar 7, 2022

Conversation

Clockwork-Muse
Copy link
Contributor

Use RegexGenerator where possible in tests.

Also added to System.Private.DataContractSerialization

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 4, 2022
@ghost
Copy link

ghost commented Mar 4, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Mar 4, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

Use RegexGenerator where possible in tests.

Also added to System.Private.DataContractSerialization

Author: Clockwork-Muse
Assignees: -
Labels:

area-Meta, community-contribution

Milestone: -

Copy link
Member

@stephentoub stephentoub 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 what's obviously a lot of work on this.

I starting going through it and left some comments, but the short of it is that I think most of the changes in here should be reverted, and we should only keep:

  1. The src changes for DataContractSerializer
  2. Changes where you made better use of built-in xunit helpers, like places you switched to use Assert.Matches(...) rather than Assert.True(Regex.IsMatch(...))

Many of the other changes to test projects won't work because those test projects also build for downlevel surface area, and the regex generator requires .NET 7 or newer surface area. And for others that do work, I think many of the changes negatively impact the clarity of the tests, and I think that's more important than any startup benefits that might come from using the source generator in the tests. The other benefit would be if we think these uses of the source generator are filling gaps from our dedicated regex test suite, but if that's the case, we should augment the test suite.

@Clockwork-Muse Clockwork-Muse marked this pull request as ready for review March 6, 2022 22:18
@Clockwork-Muse
Copy link
Contributor Author

@stephentoub - Run seems to have failed due to OOM, but I'm not sure which/if I should just launch an azp run.

@danmoseley
Copy link
Member

The out of memory is #65791.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@danmoseley danmoseley merged commit 8d12bc1 into dotnet:main Mar 7, 2022
@Clockwork-Muse Clockwork-Muse deleted the regex_gen_62105 branch March 8, 2022 01:45
@ghost ghost locked as resolved and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants