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] Complete NUnit 4 future-proofing #14878

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 9, 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

Finishes future-proofing for a potential NUnit 4 upgrade. With this, I upgraded locally to NUnit 4.x and the tests ran and passed.

Motivation and Context

Fixes #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

enhancement, tests


Description

  • Updated NUnit assertions in DevToolsProfilerTest.cs to ensure StartTime, EndTime, and TimeDeltas are non-zero, enhancing test robustness.
  • Improved message formatting in DevToolsTestFixture.cs by using string interpolation for Assert.Ignore.

Changes walkthrough 📝

Relevant files
Enhancement
DevToolsProfilerTest.cs
Update NUnit assertions for profiler validation                   

dotnet/test/common/DevTools/DevToolsProfilerTest.cs

  • Changed assertions for StartTime and EndTime to check for non-zero
    values.
  • Updated assertion for TimeDeltas to check for non-zero values.
  • +3/-3     
    DevToolsTestFixture.cs
    Use string interpolation for ignore message                           

    dotnet/test/common/DevTools/DevToolsTestFixture.cs

  • Modified Assert.Ignore message formatting to use string interpolation.

  • +1/-1     

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 9, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Test Coverage
    The test now checks for non-zero values instead of non-null. Verify this is the correct validation for profiler timing data.

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 9, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Remove unreachable code that follows a test assertion that always throws an exception

    The return statement after Assert.Ignore is unreachable code since Assert.Ignore
    throws an exception. Remove the redundant return statement.

    dotnet/test/common/DevTools/DevToolsTestFixture.cs [42-43]

     Assert.Ignore($"{EnvironmentManager.Instance.Browser} does not support Chrome DevTools Protocol");
    -return;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies that the return statement after Assert.Ignore is redundant since Assert.Ignore always throws an exception. Removing it improves code clarity and eliminates dead code.

    7
    Simplify collection validation by using LINQ instead of explicit iteration

    The foreach loop for TimeDeltas contains only an assertion without any other logic.
    Consider using LINQ's All() method for a more concise and idiomatic check.

    dotnet/test/common/DevTools/DevToolsProfilerTest.cs [137-140]

    -foreach (var delta in profiler.TimeDeltas)
    -{
    -    Assert.That(delta, Is.Not.Zero);
    -}
    +Assert.That(profiler.TimeDeltas.All(delta => delta != 0), Is.True, "All time deltas should be non-zero");
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion proposes a more concise LINQ-based approach which is idiomatic in C#. While functionally equivalent, it's primarily a stylistic improvement that makes the code more readable but doesn't affect functionality.

    5

    💡 Need additional feedback ? start a PR chat

    @@ -131,12 +131,12 @@ private void ValidateProfile(CurrentCdpVersion.Profiler.Profile profiler)
    {
    Assert.That(profiler, Is.Not.Null);
    Assert.That(profiler.Nodes, Is.Not.Null);
    Assert.That(profiler.StartTime, Is.Not.Null);
    Assert.That(profiler.EndTime, Is.Not.Null);
    Assert.That(profiler.StartTime, Is.Not.Zero);
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Modern NUnit comes with some analyzers which explained that profiler.StartTime, profiler.EndTime, and delta are value types (all doubles) so can never be null. These tests pass if we assert instead that they are != 0

    Copy link
    Member

    @nvborisenko nvborisenko Dec 11, 2024

    Choose a reason for hiding this comment

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

    Not clear what zero means. If we are in context of datetimes, let's try to be specific.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    So I took a deeper look at this, and although it's storing datetime values, it does so as unix timestamps (and therefore, numbers).

    image
    image

    NUnit 4 introduces an Is.(Not.)Default assertion, which is all we're really testing here (making sure the field is populated), but we don't have access to that yet. Either way, I hope with this clarification we can accept Is.Not.Zero as a meaningful assertion. Let me know if there's something else we should do (maybe parse to DateTime from unit timestamp?)

    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.

    [dotnet] Future-proof for NUnit 4.x upgrade
    2 participants