Skip to content

Conversation

@IT-VBFK
Copy link
Contributor

@IT-VBFK IT-VBFK commented Nov 7, 2022

IMPORTANT

  • 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.

Closes #2029

@IT-VBFK IT-VBFK changed the title [WIP] Add BeCloseTo() to TimeOnlyAssertions [WIP] Add BeCloseTo() / NotBeCloseTo() to TimeOnlyAssertions Nov 7, 2022
@IT-VBFK IT-VBFK force-pushed the issue-2029 branch 3 times, most recently from b2d887a to 713b5f9 Compare November 7, 2022 21:43
@coveralls
Copy link

coveralls commented Nov 7, 2022

Pull Request Test Coverage Report for Build 3542928324

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 96.874%

Files with Coverage Reduction New Missed Lines %
Src/FluentAssertions/Extensions/FluentTimeSpanExtensions.cs 1 96.15%
Totals Coverage Status
Change from base Build 3509527656: 0.004%
Covered Lines: 12435
Relevant Lines: 12682

💛 - Coveralls

@IT-VBFK IT-VBFK marked this pull request as ready for review November 8, 2022 17:06
@IT-VBFK IT-VBFK changed the title [WIP] Add BeCloseTo() / NotBeCloseTo() to TimeOnlyAssertions Add BeCloseTo() / NotBeCloseTo() to TimeOnlyAssertions Nov 8, 2022
Copy link
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

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

I can't figure out what kind of test branch coverage thinks we're missing 🤔

@IT-VBFK IT-VBFK requested a review from jnyrup November 19, 2022 19:37
@IT-VBFK
Copy link
Contributor Author

IT-VBFK commented Nov 19, 2022

I can't figure out what kind of test branch coverage thinks we're missing 🤔

Me too 😅

@jnyrup
Copy link
Member

jnyrup commented Nov 21, 2022

I noticed that while TimeSpan, DateTime and DateTimeOffset are linear in time, TimeOnly is circular like a clock on the wall.
So it does not have overflow like the other, but will wrap around.

This bring the question, is 23:59 is close to 00:01 ±2 minutes?
I found out that TimeOnly.IsBetween exists for this.

@IT-VBFK
Copy link
Contributor Author

IT-VBFK commented Nov 21, 2022

This bring the question, is 23:59 is close to 00:01 ±2 minutes?

Hmm.. good catch.. I would say yes.. 🤔

What does @dennisdoomen thinks about that?

@IT-VBFK
Copy link
Contributor Author

IT-VBFK commented Nov 21, 2022

I found out that TimeOnly.IsBetween exists for this.

Yes, but this is exclusive the end time. so IsBetween(23:00, 01:00) would fail if the precision includes 01:00. Is that what we want here, because DateTime.BeCloseTo() includes the maximum end date / time. IMHO this behavior should be the same at all assertions.

I could workaround by adding 1 tick to the maximum time.. so this should work.. But looks like a bad hack..

@jnyrup
Copy link
Member

jnyrup commented Nov 21, 2022

Adding 1 tick was also my idea.
That we're using IsBetween is an implementation detail.

@IT-VBFK
Copy link
Contributor Author

IT-VBFK commented Nov 21, 2022

Ok.. this should be easy..

But this does not help to get rid of the min/max calculation.

Changing the code signature to BeCloseTo(startTime, endTime) would face this issue, but I do not like having 2 different signatures for DateTimeand TimeOnly..

Edit: BeCloseTo(startTime, endTime) would definitely not work 🙈

@dennisdoomen
Copy link
Member

This bring the question, is 23:59 is close to 00:01 ±2 minutes?

Given the behavior of TimeOnly, I think this is fair.

@IT-VBFK IT-VBFK requested review from dennisdoomen and jnyrup and removed request for jnyrup November 21, 2022 19:51
@IT-VBFK
Copy link
Contributor Author

IT-VBFK commented Nov 21, 2022

Pull Request Test Coverage Report for Build 3517626725

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 11 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 95.928%

Files with Coverage Reduction New Missed Lines %
Src/FluentAssertions/Data/DataEquivalencyAssertionOptions.cs 11 86.73%
Totals Coverage Status
Change from base Build 3504387639: 0.3%
Covered Lines: 8494
Relevant Lines: 8741

💛 - Coveralls

What's the best way to generate random numbers: let coveralls calculate your coverage.. 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add .BeCloseTo()/.NotBeCloseTo() to TimeOnlyAssertions and DateOnlyAssertions

5 participants