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 3 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 9185874020

Details

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

Totals Coverage Status
Change from base Build 9161168279: 0.004%
Covered Lines: 12078
Relevant Lines: 12262

💛 - 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 718
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?

Copy link
Member

@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

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

Not all previous comments have been addressed either.

Copy link

github-actions bot commented May 15, 2024

Qodana for .NET

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@dennisdoomen dennisdoomen requested a review from jnyrup May 19, 2024 08:34
Copy link
Member

@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

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

Nice work

@ITaluone
Copy link
Contributor

@Meir017 FYI we usually use rebasing instead of merging in changes from develop ;)

@dennisdoomen
Copy link
Member

And there are some Qodana issues to address.

@Meir017 Meir017 force-pushed the feature/api-subset-and-superset branch from d2ee717 to 817afca Compare May 22, 2024 05:03
@Meir017
Copy link
Member Author

Meir017 commented May 22, 2024

@dennisdoomen done

Comment on lines +38 to +42
public void A_collection_with_all_items_of_a_superset_but_has_extra_items_is_not_a_proper_subset()
{
// Arrange
var subset = new[] { 1, 2, 3, 4 };
var superset = new[] { 4, 3, 2, 1 };
Copy link
Member

Choose a reason for hiding this comment

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

I don't know that the "extra items" the method name mentions are?

}

/// <summary>
/// Asserts that the collection is a proper subset of the <paramref name="expectedProperSubset" />.
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
/// Asserts that the collection is a proper subset of the <paramref name="expectedProperSubset" />.
/// Asserts that the collection is a proper superset of the <paramref name="expectedProperSubset" />.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

/// </content>
public partial class CollectionAssertionSpecs
{
public class BeSupersetOf
Copy link
Member

Choose a reason for hiding this comment

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

We're missing test on how BeSupersetOf should behave when Subject is either null or empty.

Comment on lines +71 to +83
[Fact]
public void A_collection_is_not_a_superset_of_an_empty_collection()
{
// Arrange
var collection = new[] { 1, 2, 3 };

// Act
Action act = () => collection.Should().BeSupersetOf(new int[0]);

// Assert
act.Should().Throw<ArgumentException>().WithMessage(
"Cannot verify containment against an empty collection*");
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, I didn't think about this case when I suggested forwarding BeProperSupersetOf to Contain.

The empty set is a valid set and { 1, 2, 3 } is a super set of {}.

I think all the set theoretic methods should handle empty sets.

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
6 participants