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

Switch to NUnit v4 #7504

Merged
merged 6 commits into from
Sep 30, 2024
Merged

Switch to NUnit v4 #7504

merged 6 commits into from
Sep 30, 2024

Conversation

rubo
Copy link
Contributor

@rubo rubo commented Sep 28, 2024

Changes

Migrated to NUnit v4:

  • Replaced Assert.<Method>() assertions with Assert.That() assertions
  • Replaced CollectionAssert assertions with Assert assertions
  • Replaced TestContext.WriteLine withTestContext.Out.Write
  • Replaced obsolete TimeoutAttribute with MaxTimeAttribute

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Remarks

With this upgrade, the FluentAssertions package doesn't seem needed.

@rubo rubo marked this pull request as ready for review September 28, 2024 10:17
@LukaszRozmej
Copy link
Member

Will CancelAfter fail the test?

@rubo rubo marked this pull request as draft September 28, 2024 12:56
@rubo
Copy link
Contributor Author

rubo commented Sep 28, 2024

Will CancelAfter fail the test?

@LukaszRozmej That's a good question. It won't. Actually, as the Timeout is gone, the replacement is CancelAfter but it works differently. Actually, there are 2 options—MaxTime and CancelAfter. MaxTime fails the test if the test exceeds the time limit. But it lets the test run as long as it takes and then checks the actual duration. CancelAfter is for canceling long-running tasks with a cancellation token, but most of our test APIs don't support that.
With this said, we better use MaxTime instead of CancelAfter.

@LukaszRozmej
Copy link
Member

Maybe we should add both?

@rubo
Copy link
Contributor Author

rubo commented Sep 28, 2024

Maybe we should add both?

In some cases, yes. But our API doesn't support it. Modifying it would bring many cascading changes, which I'd like to avoid in this PR. Another issue is overkill with timeouts. Check this out, for instance. Why do we need a timeout there? There are a bunch of such examples.

@rubo rubo marked this pull request as ready for review September 28, 2024 20:20
@rubo rubo merged commit f825abd into master Sep 30, 2024
66 checks passed
@rubo rubo deleted the feature/nunit-update branch September 30, 2024 20:06
rjnrohit pushed a commit that referenced this pull request Oct 10, 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