Skip to content

[stdlib] Fix and Improve Unit Tests for Set #24575

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 3 commits into from
May 9, 2019

Conversation

palimondo
Copy link
Contributor

@palimondo palimondo commented May 7, 2019

Analyzing how well the #21300 handles isEmpty cases, I've noticed that the unit test that should be testing Set.Sequence variants of methods:

  • isSubset
  • isSuperSet
  • isStrictSubset
  • isStrictSuperSet

contained errors (sometimes testing Set.Set variants) and the tests did not fully cover the potential edge cases which might arise when implementing performance optimizations for the empty cases.

This PR fixes these errors and adds equivalent coverage for both Set.Set and Set.Sequence variants.

palimondo added 2 commits May 7, 2019 19:35
Systematically test also set algebra:
* with empty sets on both sides
* inverse operation
Really test the Sequence variants of set algebra and cover the same cases as Set.Set.
@palimondo
Copy link
Contributor Author

@swift-ci please test

@palimondo palimondo requested a review from dabrahams May 7, 2019 22:31
Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Nice catch! These are good additions; I only have the small nit below.

Use explicit type annotation to make it crystal clear (beyoud any reasonable doubt) these are indeed testing the Sequence variants.
@palimondo
Copy link
Contributor Author

@swift-ci please test

@palimondo palimondo merged commit 14f250b into swiftlang:master May 9, 2019
@palimondo palimondo deleted the set-seq-unittests branch May 9, 2019 14:39
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.

2 participants