Skip to content

Add support for DateTime comparison with TimeSpan tolerance #4618

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

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

RenderMichael
Copy link
Contributor

Fixes #4600

@RenderMichael
Copy link
Contributor Author

RenderMichael commented Feb 11, 2024

I had to pull out the test data into a source method because DateTime/TimeSpan are not allowed in attributes.

Also, unlike the existing double and ints being tested, DateTime's formatting in the error messages is different from the normal ToString(), so I called MsgUtils.FormatValue directly like the exception message formatting itself does.

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

Thanks @RenderMichael.

The code changes are nice and simple.

However as it also affects other areas where Tolerance is used, could you also please update the other constraint tests: GreaterThanOrEqualConstraintTests, LessThanConstraintTests and LessThanOrEqualConstraintTests by adding use cases for TimeSpan.

@RenderMichael
Copy link
Contributor Author

Thanks for the feedback @manfred-brands

The PR has been updated, I also added an equality test to the existing Greater Than tests.

@manfred-brands manfred-brands self-requested a review February 11, 2024 06:34
Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

Thanks again @RenderMichael for adding those extra tests.

Nothing more from me.

@RenderMichael
Copy link
Contributor Author

Sorry for the update after the fact, I noticed I used some terrible variable names in the tolerance method. Fixed with no functional changes

Copy link
Member

@stevenaw stevenaw left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @RenderMichael ! Appreciate the final cleanup you did too :)

@stevenaw
Copy link
Member

I'm going to try converting this to a draft and back to see if it un-sticks the WIP check

@stevenaw stevenaw marked this pull request as draft February 15, 2024 13:14
@stevenaw stevenaw marked this pull request as ready for review February 15, 2024 13:15
@stevenaw
Copy link
Member

@OsirisTerje The WIP check seems stuck here and I don't seem to be able to override the merge. Are you able to help?

@OsirisTerje OsirisTerje merged commit 56fa6f4 into nunit:master Feb 15, 2024
@OsirisTerje
Copy link
Member

@stevenaw Merged.

@stevenaw
Copy link
Member

Thanks @OsirisTerje

@RenderMichael RenderMichael deleted the tolerance branch February 15, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DateTime/TimeSpan support for inequality tolerance
4 participants