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

[dotnet] Migrate NUnit assertions to Assert.That syntax #14853

Merged
merged 11 commits into from
Dec 6, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 4, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Migrate NUnit assertions to Assert.That syntax

Motivation and Context

Contributes to #14852

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Tests


Description

  • Migrated AlertsTest and ChildrenFindingTest to use modern NUnit syntax.
  • Replaced Assert.AreEqual with Assert.That for better readability and flexibility.
  • Improved collection assertions using Has.Count.EqualTo.
  • Enhanced exception assertions with Throws.InstanceOf.
  • Utilized Assert.Multiple for grouping multiple assertions.

Changes walkthrough 📝

Relevant files
Tests
AlertsTest.cs
Migrate AlertsTest to modern NUnit syntax                               

dotnet/test/common/AlertsTest.cs

  • Migrated from Assert.AreEqual to Assert.That with Is.EqualTo.
  • Improved readability by using Assert.Multiple for multiple assertions.
  • Used Has.Count.EqualTo for collection count assertions.
  • Enhanced exception assertions with Throws.InstanceOf.
  • +19/-19 
    ChildrenFindingTest.cs
    Migrate ChildrenFindingTest to modern NUnit syntax             

    dotnet/test/common/ChildrenFindingTest.cs

  • Replaced Assert.AreEqual with Assert.That using Is.EqualTo.
  • Utilized Has.Count.EqualTo for collection assertions.
  • Improved exception handling assertions with Throws.InstanceOf.
  • Enhanced readability with Assert.Multiple for grouped assertions.
  • +65/-43 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 4, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    Multiple test methods follow very similar patterns of setting up alerts, checking titles and text. Consider extracting common setup and assertion logic into helper methods to reduce duplication.

    Test Organization
    Related test cases for finding elements by different locator strategies are scattered. Consider grouping related tests together (e.g., all CSS selector tests, all XPath tests) for better organization and maintainability.

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 4, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Group related assertions using Assert.Multiple to improve test readability and failure reporting

    Use Assert.Multiple to group related assertions for parent.FindElements() calls to
    improve test readability and error reporting.

    dotnet/test/common/ChildrenFindingTest.cs [345-346]

    -Assert.That(parent.FindElements(By.TagName("div")), Has.Count.EqualTo(2));
    -Assert.That(parent.FindElements(By.TagName("span")), Has.Count.EqualTo(2));
    +Assert.Multiple(() => {
    +    Assert.That(parent.FindElements(By.TagName("div")), Has.Count.EqualTo(2));
    +    Assert.That(parent.FindElements(By.TagName("span")), Has.Count.EqualTo(2));
    +});
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: Using Assert.Multiple for related assertions is a good practice that improves test readability and provides better error reporting by showing all failures at once, but it's a minor enhancement that doesn't affect functionality.

    4

    💡 Need additional feedback ? start a PR chat

    @RenderMichael RenderMichael changed the title [dotnet] Start migrating to modern NUnit syntax [dotnet] Migrate NUnit assertions to Assert.That syntax Dec 5, 2024
    @RenderMichael
    Copy link
    Contributor Author

    This PR is ready. Like you said, big boom.

    @nvborisenko
    Copy link
    Member

    NP, easy to review. CI doesn't like extra whitespaces in code style.

    @RenderMichael
    Copy link
    Contributor Author

    @nvborisenko Sorry for the whitespace! It should work now.

    @RenderMichael
    Copy link
    Contributor Author

    RenderMichael commented Dec 6, 2024

    Not sure how this slipped by me again. I just ran scripts\format.sh locally and it passed, so this time for sure...

    @nvborisenko
    Copy link
    Member

    Turbo-scroll mode has been activated... Thank you, @RenderMichael, for this modernization!

    @nvborisenko nvborisenko merged commit ae6fe72 into SeleniumHQ:trunk Dec 6, 2024
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the dotnet-modern-nunit branch December 6, 2024 17:38
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Dec 7, 2024
    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.

    2 participants