date time range assertions - asserting difference is not negative by jon-is-me · Pull Request #1313 · fluentassertions/fluentassertions

jon-is-me · 2020-04-19T00:04:28Z

IMPORTANT

The code complies with the Coding Guidelines for C#.
The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

Fixing issue where assertion on DateTimeRange will fail on BeLessThan or BeWithin when the subject and target has a negative delta.

Reported in #1165

jnyrup · 2020-04-19T10:06:17Z

Thanks for looking in this issue 👍

I like the idea about replacing differs with the explicit before/after.
It's more human-friendly to read before by 10s, than differs by -10s.

Could this be simplified by moving the actual >= TimeSpan.Zero into the dictionary of predicates
E.g.

ts1 >= TimeSpan.Zero && ts1 > ts2

Then if IsMatched fails, whether actual is positive or negative can control whether the failure message should be but it is after or but it is before.

dennisdoomen

Great work. I just have a couple of wording/terminology remarks to address.

And would you be willing to also update the release notes at https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md?

Src/FluentAssertions/Primitives/DateTimeOffsetRangeAssertions.cs

Src/FluentAssertions/Primitives/DateTimeRangeAssertions.cs

dennisdoomen · 2020-04-19T16:12:41Z

Tests/FluentAssertions.Specs/Primitives/DateTimeAssertionSpecs.cs

        }

+        [Fact]
+        public void When_asserting_subject_be_morethan_10_seconds_after_target_but_subject_is_before_target_should_throw()


(Nitpicking) Missing _ between more and than

I was debating if i should have a be_more_than or bemorethan because the method name is BeMoreThan but somehow I went in the middle lol. I'll put an underscore to between the words.

Don’t refer to technical names such as the names of methods, variables and UI elements in the name of a test. In my opinion, the name of a test should explain the functional scenario, whereas the cause and effect should clearly illustrate the technicalities of that scenario. If some class or member is renamed, it shouldn’t affect the functional name of the test.

From https://www.continuousimprover.com/2015/11/12-tips-to-write-unit-tests-that-dont.html

In other words, I never use the literal name of a specific method in a test name.

dennisdoomen · 2020-04-19T16:14:35Z

Tests/FluentAssertions.Specs/Primitives/DateTimeAssertionSpecs.cs

+        public void When_asserting_subject_be_morethan_10_seconds_after_target_but_subject_is_before_target_should_throw()
+        {
+            // Arrange
+            var target = 1.January(0001).At(0, 0, 30);


Instead of target we use the term expectation everywhere.

@dennisdoomen We do use target elsewhere in DateTimeOffsetAssertionSpecs.cs and DateTimeAssertionSpecs.cs.
@jonathann92 If you change this to expectation, I'll clean up the others.

@jnyrup I can change target to expectation in the files I touched. Would you guys also like to me to change it in DateTimeOffsetRangeAssertions.cs and DateTimeOffsetRangeAssertions.cs?

jon-is-me · 2020-04-19T23:28:43Z

@dennisdoomen

I think it should be something like but it is behind by.

Instead of after by, use ahead by

My reasoning for using before and after is that its the same wording as the method name.

Also ahead can mean "before" in some contexts other than time and this can lead to confusion. For example, there are 2 people in a line:

The 1st person is before the 2nd person
The 1st person is ahead of the 2nd person

Did you still want it be behind and ahead instead of before and after?

dennisdoomen · 2020-04-20T05:34:56Z

but it is before by 2 ms

I'm not a native speaker, but somehow it feels wrong. I'm wondering what is the correct English wording. Giving you're from CA, I guess you should know better than me.

jnyrup · 2020-04-20T05:44:58Z

The no native speaker disclaimer applies to me as well.

"but it is before by 2 ms" sounds like a mix of
"but it is behind by 2 ms" and "but it is 2 ms before".

I think behind/ahead is used with time if e.g. your watch is out of sync, then
"it is behind by 2s".

jon-is-me · 2020-04-23T01:45:00Z

Ok it makes sense when we're talking about times. I changed the messaging to ahead/behind instead of after/before.

dennisdoomen · 2020-04-23T06:35:14Z

Are you willing to also update the release notes?

jnyrup

This looks great!
Only a few minor comments from here

jnyrup · 2020-04-23T09:41:52Z

Tests/FluentAssertions.Specs/Primitives/DateTimeAssertionSpecs.cs

+        public void When_asserting_subject_be_morethan_10_seconds_after_target_but_subject_is_before_target_should_throw()
+        {
+            // Arrange
+            var target = 1.January(0001).At(0, 0, 30);


@dennisdoomen We do use target elsewhere in DateTimeOffsetAssertionSpecs.cs and DateTimeAssertionSpecs.cs.
@jonathann92 If you change this to expectation, I'll clean up the others.

jnyrup · 2020-04-23T09:43:38Z

Tests/FluentAssertions.Specs/Primitives/DateTimeAssertionSpecs.cs

+            var subject = 1.January(0001).At(0, 0, 15);
+
+            // Act / Assert
+            Action action = () => subject.Should().BeMoreThan(10.Seconds()).After(target);


Please structure this as

// Act Action action = ... // Assert action.Should()...

(If my regex-fu serves me well, we follow this pattern everywhere)

We do use target elsewhere in DateTimeOffsetAssertionSpecs.cs

@jnyrup legacy code ;-)

jnyrup · 2020-04-23T09:46:57Z

Tests/FluentAssertions.Specs/Primitives/DateTimeAssertionSpecs.cs

        }

+        [Fact]
+        public void When_asserting_subject_be_morethan_10_seconds_after_target_but_subject_is_before_target_should_throw()


Suggested change

public void When_asserting_subject_be_morethan_10_seconds_after_target_but_subject_is_before_target_should_throw()

public void When_asserting_subject_to_be_more_than_10_seconds_after_target_but_subject_is_before_target_it_should_throw()

I'll clean up the other methods not using it_should_throw

jnyrup · 2020-04-23T09:51:14Z

Src/FluentAssertions/Primitives/DateTimeRangeAssertions.cs

            return new AndConstraint<TAssertions>(parentAssertions);
        }
+
+        private static string PositionRelativeToTarget(DateTimeOffset actual, DateTimeOffset target)


Suggested change

private static string PositionRelativeToTarget(DateTimeOffset actual, DateTimeOffset target)

private static string PositionRelativeToTarget(DateTime actual, DateTime target)

jon-is-me · 2020-04-23T14:32:22Z

Are you willing to also update the release notes?

Does this wording sound right?
Asserting DateTime(Offset) `Before` will fail when subject is after and vice versa

jnyrup · 2020-04-26T18:08:26Z

@jonathann92 I would write it in past tense, since it's now fixed.
@dennisdoomen Any comments on how to formulate this in the release notes?

dennisdoomen · 2020-04-27T10:41:32Z

Any comments on how to formulate this in the release notes?

Taking from the original bug report:

Ensures that date time assertions like "a is less than an hour after b" don't succeed when `a - b == -30.Minutes()`

jnyrup · 2020-04-29T05:43:09Z

@jonathann92 again, thanks for taking the time to look this issue and contributing a fix ❤️

dennisdoomen · 2020-04-29T05:59:59Z

Kudos for @jonathann92 indeed. With all these contributions, 6.0 is going to be awesome.

date time range assertions - asserting difference is not negative

22ec242

jon-is-me mentioned this pull request Apr 19, 2020

DateTime Should().BeLessThan(...).After(b) not check After #1165

Closed

dennisdoomen reviewed Apr 19, 2020

View reviewed changes

using ahead/behind instead of after/before

2d8526a

dennisdoomen approved these changes Apr 23, 2020

View reviewed changes

jnyrup requested changes Apr 23, 2020

View reviewed changes

jnyrup linked an issue Apr 26, 2020 that may be closed by this pull request

DateTime Should().BeLessThan(...).After(b) not check After #1165

Closed

jon-is-me added 3 commits April 28, 2020 19:44

Adding release notes for date time range fix

8f7db73

Change to DateTime for helper method in DateTimeRangeAssertions

72cd7d9

Update unit tests to have same structure as other tests

7aead48

jon-is-me force-pushed the datetimerange-negative-delta-fix branch from b5ed3a5 to 7aead48 Compare April 29, 2020 02:46

jnyrup approved these changes Apr 29, 2020

View reviewed changes

jnyrup merged commit a9b9518 into fluentassertions:develop Apr 29, 2020

jnyrup mentioned this pull request Feb 21, 2021

DateTimeAssertions.BeWithin and BeLessThan do not work if date is on the opposite side of reference date in .Before/.After #1467

Closed

	public void When_asserting_subject_be_morethan_10_seconds_after_target_but_subject_is_before_target_should_throw()
	public void When_asserting_subject_to_be_more_than_10_seconds_after_target_but_subject_is_before_target_it_should_throw()

	private static string PositionRelativeToTarget(DateTimeOffset actual, DateTimeOffset target)
	private static string PositionRelativeToTarget(DateTime actual, DateTime target)

Conversation

jon-is-me commented Apr 19, 2020

IMPORTANT

Uh oh!

jnyrup commented Apr 19, 2020

Uh oh!

dennisdoomen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-is-me commented Apr 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisdoomen commented Apr 20, 2020

Uh oh!

jnyrup commented Apr 20, 2020

Uh oh!

jon-is-me commented Apr 23, 2020

Uh oh!

dennisdoomen commented Apr 23, 2020

Uh oh!

jnyrup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-is-me commented Apr 23, 2020

Uh oh!

jnyrup commented Apr 26, 2020

Uh oh!

dennisdoomen commented Apr 27, 2020

Uh oh!

jnyrup commented Apr 29, 2020

Uh oh!

dennisdoomen commented Apr 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jon-is-me commented Apr 19, 2020 •

edited

Loading