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

Feature: Add collections methods BeProperSubsetOf / BeProperSupersetOf / BeSupersetOf #2432

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Meir017
Copy link
Member

@Meir017 Meir017 commented Nov 1, 2023

resolves #2363

IMPORTANT

  • If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
  • If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
  • If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
  • If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
    • Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

Copy link

github-actions bot commented Nov 1, 2023

Qodana for .NET

4 new problems were found

Inspection name Severity Problems
Possible multiple enumeration 🔶 Warning 4

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/qodana-action@v2023.2.8
        with:
          upload-result: true
Contact Qodana team

Contact us at qodana-support@jetbrains.com

@Meir017 Meir017 force-pushed the feature/api-subset-and-superset branch from 5f23897 to 398bb8f Compare November 1, 2023 17:20
@coveralls
Copy link

coveralls commented Nov 1, 2023

Pull Request Test Coverage Report for Build 6756573228

  • 56 of 56 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 97.398%

Totals Coverage Status
Change from base Build 6755976903: 0.005%
Covered Lines: 11758
Relevant Lines: 11949

💛 - Coveralls

@Meir017 Meir017 changed the title Feature/api subset and superset Feature: Add collections methods BeProperSubsetOf / BeProperSupersetOf / BeSupersetOf Nov 1, 2023
@vbreuss
Copy link
Contributor

vbreuss commented Nov 3, 2023

These methods come from set theory, but work on IEnumerable<T>. What happens when duplicate entries are provided in the subject or the expected value?
I would expect duplicate entries to be ignored, right?
What happens, e.g.

var subject = new []{1, 1, 1, 2, 2, 3, 3};
subject.Should().BeProperSubsetOf(new []{1, 2, 3, 3, 3, 4});

Should we add test cases for it?

@Meir017
Copy link
Member Author

Meir017 commented Nov 3, 2023

@vbreuss done! thanks for the suggestion

Comment on lines +708 to +712
AssertBeSubsetOf(expectedProperSuperset, "proper subset", because, becauseArgs);

ISet<T> expectedItems = expectedProperSuperset.ConvertOrCastToSet();

if (expectedItems.Intersect(Subject).Count() == expectedItems.Count)
Copy link
Member

Choose a reason for hiding this comment

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

Could this cause multiple enumeration of either Subject or expectedProperSuperset?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, maybe we should create an ISet and an ICollection for the Subject and expected values?

Comment on lines +80 to +81
"Expected superset to be a proper superset of {4, 3, 2, 1} because we want to test the failure message, " +
"but items {1, 1, 1, 2, 2, 3, 3, 4} are equivalent to the superset");
Copy link
Member

Choose a reason for hiding this comment

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

Should we print the non-set version of expected?
This might be useful when object.Equals(object) and object.ToString() are not aligned as they are for int, i.e. where ToString() includes more/different information than what Equals use to compare instances.

Copy link
Member Author

@Meir017 Meir017 Nov 4, 2023

Choose a reason for hiding this comment

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

hmm, maybe we should create an ISet and an ICollection for the Subject and expected values?
then we could print both versions of the values, both the unique values and the raw values,
does this make sense?

@@ -41,6 +41,9 @@ collection.Should().EndWith(8);
collection.Should().EndWith(new[] { 5, 8 });

collection.Should().BeSubsetOf(new[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, });
collection.Should().BeProperSubsetOf(new[] { 1, 2, 5, 6, 7, 8 });
Copy link
Member

Choose a reason for hiding this comment

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

🤔 I would add some comments here to explain the difference (again) between a normal subset/superset and a proper one.

Comment on lines +15 to +18
* Introduced new collection assertions methods - [#2432](https://github.com/fluentassertions/fluentassertions/pull/2432)
* `BeProperSubsetOf`
* `BeProperSupersetOf`
* `BeSupersetOf`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Introduced new collection assertions methods - [#2432](https://github.com/fluentassertions/fluentassertions/pull/2432)
* `BeProperSubsetOf`
* `BeProperSupersetOf`
* `BeSupersetOf`
* Introduced new collection assertions methods `BeProperSubsetOf`, `BeProperSupersetOf` and `BeSupersetOf - [#2432](https://github.com/fluentassertions/fluentassertions/pull/2432)
* `BeSupersetOf`

{
Execute.Assertion
.BecauseOf(because, becauseArgs)
.FailWith("Expected {context:collection} to be a proper subset of {0}{reason}, but items {1} are equivalent to the subset",
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Can we make the last part of the failure clearer?

@@ -3262,6 +3298,62 @@ public AndConstraint<TAssertions> StartWith(T element, string because = "", para
return StartWith(new[] { element }, ObjectExtensions.GetComparer<T>(), because, becauseArgs);
}

internal void AssertContainment(IEnumerable<T> expected, string containmentType, string because = "", params object[] becauseArgs)
Copy link
Member

Choose a reason for hiding this comment

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

🙃 We prefer to keep private methods just below the last usage.
🔧 Make it private

public class BeProperSubsetOf
{
[Fact]
public void When_collection_is_proper_subset_of_a_specified_collection_it_should_not_throw()
Copy link
Member

Choose a reason for hiding this comment

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

🔧 We prefer to avoid using terms like when and should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BeProperSubsetOf and BeProperSupersetOf
5 participants