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.

Sign up for GitHub

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

Jump to bottom

KeyAnalyzer tests. #90301

Merged
merged 2 commits into from
Oct 20, 2023
Merged

KeyAnalyzer tests. #90301

merged 2 commits into from
Oct 20, 2023

Conversation

IDisposable
Copy link
Contributor

@IDisposable IDisposable commented Aug 10, 2023 •
edited
Loading

This is based extracting the unit tests for KeyAnalyzer and a couple trivial code simplifications without the huge changes in PR #89863

Hoisted the calculation of the maximum collisions allow outside the HasSufficientUniquenessFactor as the calculation only needs to be done once (also makes it more testable).

Made ContainsAnyLetters and SufficientUniquenessFactor internal so they can be tested.

Added tests for ContainsAnyLetters and SufficientUniquenessFactor are working correctly.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 10, 2023
@ghost
Copy link

ghost commented Aug 10, 2023

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

Issue Details

Hoisted the calculation of the maximum collisions allow outside the
HasSufficientUniquenessFactor as the calculation only needs
to be done once (also makes it more testable).

Hoisted the calculation of the maximum offset outside of the offset
loops (as it's only dependent on the slice count and minimum length).

Made ContainsAnyLetters and SufficientUniquenessFactor
internal so they can be tested.

Added tests for ContainsAnyLetters and SufficientUniquenessFactor are working correctly.

Author: IDisposable
Assignees: -
Labels:

area-System.Collections, community-contribution
Milestone: -

@IDisposable IDisposable mentioned this pull request Aug 10, 2023
Closed
@IDisposable IDisposable marked this pull request as ready for review August 10, 2023 09:15
@build-analysis build-analysis bot mentioned this pull request Aug 10, 2023
Closed
@stephentoub
Copy link
Member

Benchmarks are identical

Can we please undo the hoisting-related changes then? Let's keep this just tests.

@IDisposable
Copy link
Contributor Author

IDisposable commented Aug 10, 2023 •
edited
Loading

Removed the one that isn't needed for testing.

@IDisposable
Add some KeyAnalyzer tests.
4f7dbe4 
Made ContainsAnyLetters and SufficientUniquenessFactor
internal so they can be tested.

Hoisted the calculation of the maximum collisions allowed outside the
HasSufficientUniquenessFactor for testability (the calculation only
needs to be done once).

Added tests for ContainsAnyLetters and SufficientUniquenessFactor are
working correctly.
@IDisposable IDisposable force-pushed the test-key-analyzer branch 2 times, most recently from ba0c068 to 4f7dbe4 Compare August 10, 2023 16:21
@IDisposable IDisposable changed the title KeyAnalyzer tests and small tweaks. KeyAnalyzer tests. Aug 10, 2023
@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Aug 14, 2023
adamsitnik
adamsitnik reviewed Oct 19, 2023
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

The test is failing, but I think I've found the reason. I am going to apply my suggestion and merge it if the CI gets green.

src/libraries/System.Collections.Immutable/tests/Frozen/KeyAnalyzerTests.cs Outdated Show resolved Hide resolved
@IDisposable
Copy link
Contributor Author

Yep, that was from when it was still using my other KeyAnalyzer changes (that resulted in an empty set when HasSufficientUniquenessFactor returned false).

Since we're not using that code right now, the test was wrong, sorry.

Thanks for fixing it.

adamsitnik
adamsitnik approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @IDisposable !

@adamsitnik adamsitnik merged commit 0151c46 into dotnet:main Oct 20, 2023
109 checks passed
@IDisposable IDisposable deleted the test-key-analyzer branch November 11, 2023 03:24
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adamsitnik adamsitnik adamsitnik approved these changes

Assignees
No one assigned
Labels
area-System.Collections community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@IDisposable @stephentoub @adamsitnik @eiriktsarpalis