Add Should().NotThrowAfter assertion for actions #942

frederik-h · 2018-10-08T19:49:43Z

This adds a Should().NotThrowAfter assertion for Actions which is a variant of Should().NotThrow
which asserts that the Action should stop throwing exceptions after a given wait time. See issue #940 for a discussion of this feature.

If this gets merged, it should probably also be considered to add corresponding methods for the other action assertions such as ThrowAfter etc.

IMPORTANT

The Pull Request is targeted at the master branch.
The code complies with the Coding Guidelines for C# 3.0, 4.0 and 5.0.
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 to documentation.md in this pull request so the documentation will appear on the website.

Regarding compliance to the guidelines: I was unable to run the CSharpGuidelinesAnalyzer, since I do not have access to Windows/VisualStudio at home.
Regarding the documentation: I am not sure where to put this right now; I think we can add it later.

This extends the NotThrow Action assertion by an overload which allows to specify a wait time. This asserts that the Action should stop throwing exceptions after the wait time has passed.

The overload of NotThrow with waiting time is renamed to NotThrowAfter to emphasize its purpose.

jnyrup · 2018-10-09T21:02:27Z

Src/FluentAssertions/Specialized/ActionAssertions.cs

+        /// Zero or more objects to format using the placeholders in <see cref="because" />.
+        /// </param>
+        /// <exception cref="ArgumentOutOfRangeException">Throws if waitTime or pollInterval are negative.</exception>
+        public void NotThrowAfter(int waitTime, int pollInterval, string because = "", params object[] becauseArgs)


🔧 I would prefer to use TimeSpan for waitTime and pollInterval.

🔧 Maybe NotThrowAnymoreAfter

🤔 Do we also need to be able to specify an exception type here?
🤔 Do we also need this assertion for Func<Task> for consistency with the other exception APIs?

Maybe NotThrowAnymoreAfter

Are you sure? I think that this would be a tad too long. It might also be misleading. Doesn't "anymore" suggest that the Action must have been throwing exceptions at some point

I would prefer to use TimeSpan for waitTime and pollInterval.

Sounds good. Would you keep an overload with the current signature or would you expect the user to convert from that representation to the TimeSpan?

True. Very true. Alright. Let's stick with NotThrowAfter

Would you keep an overload with the current signature or would you expect the user to convert from that representation to the TimeSpan?

They can use the TimeSpan extension methods like 12.Seconds().

Do we also need to be able to specify an exception type here?

I have not encountered a use case for this so far. This would mean that the Action could be still throwing exceptions but not the specified one ... :confused. We could add it for consistency's sake. You could also wait until someone requests it. I don't know your policy regarding these kind of questions 😏

Do we also need this assertion for Func<Task> for consistency with the other exception APIs?

Probably.

Src/FluentAssertions/Specialized/ActionAssertions.cs

Tests/Shared.Specs/ExceptionAssertionSpecs.cs

Src/FluentAssertions/Specialized/ActionAssertions.cs

Tests/Shared.Specs/ExceptionAssertionSpecs.cs

dennisdoomen · 2018-10-11T05:08:08Z

Src/FluentAssertions/Specialized/ActionAssertions.cs

+        /// Zero or more objects to format using the placeholders in <see cref="because" />.
+        /// </param>
+        /// <exception cref="ArgumentOutOfRangeException">Throws if waitTime or pollInterval are negative.</exception>
+        public void NotThrowAfter(int waitTime, int pollInterval, string because = "", params object[] becauseArgs)


🔧 Maybe NotThrowAnymoreAfter

dennisdoomen · 2018-10-11T05:09:08Z

Src/FluentAssertions/Specialized/ActionAssertions.cs

+        /// Zero or more objects to format using the placeholders in <see cref="because" />.
+        /// </param>
+        /// <exception cref="ArgumentOutOfRangeException">Throws if waitTime or pollInterval are negative.</exception>
+        public void NotThrowAfter(int waitTime, int pollInterval, string because = "", params object[] becauseArgs)


🤔 Do we also need to be able to specify an exception type here?
🤔 Do we also need this assertion for Func<Task> for consistency with the other exception APIs?

* Change waitTime and pollInterval to TimeSpan * Use Thread.Sleep instead of Task.Delay * Add better exception messages if waitTime or pollInterval are out of range * Add tests for invalid argument exceptions * Simplify tests

Tests/Shared.Specs/ExceptionAssertionSpecs.cs

Src/FluentAssertions/Specialized/ActionAssertions.cs

Tests/Shared.Specs/ExceptionAssertionSpecs.cs

Src/FluentAssertions/Specialized/ActionAssertions.cs

jnyrup · 2018-10-12T22:09:44Z

The failing build should have been fixed with #944 .

* Change local variable names to start with a lower case letter in tests of the NotThrowAfter assertion * Change exception message if NotThrowAfter

jnyrup · 2018-10-13T18:31:11Z

The build fails as Thread.Sleep is not available in .net standard earlier than 2.0.
In https://github.com/fluentassertions/fluentassertions/blob/master/Src/FluentAssertions/Specialized/ExecutionTimeAssertions.cs this is handled by wrapping the action inside a Task. Note that this to enable interrupting the Action.

The implementation of NotThrowAfter uses Thread.Sleep. This is not supported in older .net standard versions. Hence, Thread.Sleep is replaced by a ManualResetEvent which also allows to wait for a specified time.

frederik-h · 2018-10-26T12:08:57Z

The build fails as Thread.Sleep is not available in .net standard earlier than 2.0.
In https://github.com/fluentassertions/fluentassertions/blob/master/Src/FluentAssertions/Specialized/ExecutionTimeAssertions.cs this is handled by wrapping the action inside a Task. Note that this to enable interrupting the Action.

@jnyrup Did you have a chance to take a look at my attempt to solve this?

jnyrup · 2018-10-26T12:31:12Z

@frederik-h Sorry for the delayed response.
I did have a look at it and my initial gut feeling was that it felt like a hack to use a synchronization mechanism to implement a wait.

What about if we used something similar to https://github.com/fluentassertions/fluentassertions/blob/master/Src/FluentAssertions/Specialized/ExecutionTimeAssertions.cs and wrap the synchronous function inside a Task which can then be polled or re-run.
That should also have the benefit that for a long-running method, the test method will not run longer than waitTime.

frederik-h · 2018-10-29T19:13:32Z

@frederik-h Sorry for the delayed response.
I did have a look at it and my initial gut feeling was that it felt like a hack to use a synchronization mechanism to implement a wait.

Yes, I also hesitated at first, but since the waiting is part of the specified behavior of the method and not some implementation specific side effect, I decided that there is no substantial reason not to use it.

What about if we used something similar to https://github.com/fluentassertions/fluentassertions/blob/master/Src/FluentAssertions/Specialized/ExecutionTimeAssertions.cs and wrap the synchronous function inside a Task which can then be polled or re-run.
That should also have the benefit that for a long-running method, the test method will not run longer than waitTime.

I am not sure that this is a benefit, but I don't have a strong opinion on this matter. This would implement a different functionality than what I was trying to implement, but it could probably be used for more or less the same purpose. What would you do if the Task is not done after the waitTime? Would you throw an exception? Even if the subject Action did not throw any exceptions?

Src/FluentAssertions/Specialized/ActionAssertions.cs

Tests/Shared.Specs/ExceptionAssertionSpecs.cs

Src/FluentAssertions/Specialized/ActionAssertions.cs

jnyrup · 2018-11-07T18:02:02Z

@dennisdoomen do you have an opinion on the usage of ManualResetEvent for waiting?

dennisdoomen · 2018-11-07T19:10:53Z

do you have an opinion on the usage of ManualResetEvent for waiting?

He's not doing concurrent calls, is he? So a simple Thread.Sleep would suffice.

jnyrup · 2018-11-07T19:13:10Z

@dennisdoomen Thread.Sleep is not available in netstandard until 2.0, see earlier comment.

dennisdoomen · 2018-11-07T19:17:31Z

Thread.Sleep is not available in netstandard until 2.0, see earlier comment.

So maybe we should not support this method except for .NET Standard 2.0 and higher, and the full .NET Framework.

frederik-h · 2018-11-08T19:51:46Z

Thread.Sleep is not available in netstandard until 2.0, see earlier comment.

So maybe we should not support this method except for .NET Standard 2.0 and higher, and the full .NET Framework.

Sounds like the best option.

Test "NotThrowAfter_when_subject_is_async_it_should_not_throw" asserts that NotThrowAfter should throw and hence it is renamed accordingly.

The messages of the exceptions that are raised if the TimeSpan parameters of NotThrowAfter are negative claim that the parameters should be "positive". Changed to "non-negative". Further change: * Minor formatting change

This reverts commit b46d678. NotThrowAfter has to block the current thread. Since Thread.Sleep() is not available on older versions of netstandard, the previous commit introduced a workaround. It seems prefarable not to use this workaround and disable NotThrowAfter for target frameworks that don't support Thread.Sleep().

Older versions of netstandard don't support Thread.Sleep() which is required by NotThrowAfter.

Merge latest changes from upstream

frederik-h · 2018-11-19T19:46:07Z

Thread.Sleep is not available in netstandard until 2.0, see earlier comment.

So maybe we should not support this method except for .NET Standard 2.0 and higher, and the full .NET Framework.

Sounds like the best option.

I have now implemented this.

frederik-h · 2018-11-20T13:24:05Z

I don't think that my changes have caused the test failure in the test from FluentAssertions.Specs.ExecutionTimeAssertionsSpecs that led to the failure of the AppVeyor build.

dennisdoomen · 2018-11-20T15:20:29Z

I don't think that my changes have caused the test failure in the test from

No, that one is unstable.

dennisdoomen

Only minor comments. Feel free to reject them.

Src/FluentAssertions/Specialized/ActionAssertions.cs

Tests/Shared.Specs/ExceptionAssertionSpecs.cs

* Add braces to if * Invert #if condition

Add braces to single line ifs.

jnyrup · 2018-11-22T20:08:41Z

For a consistent API, we should probably also expose NotThrowAfter for AsyncFunctionAssertions and FunctionAssertions as well.
@dennisdoomen Could those be implemented in a separate PR or would that block for the master branch for potential minor releases in between?

dennisdoomen · 2018-11-22T20:19:34Z

Could those be implemented in a separate PR or would that block for the master branch for potential minor releases in between?

I think we need to be a bit more critical.

frederik-h · 2018-12-04T19:35:54Z

I think we need to be a bit more critical.

Regarding the FunctionAssertions etc. I suppose? Is there anything left to do regarding this PR? 😄

dennisdoomen · 2018-12-04T20:20:57Z

Regarding the FunctionAssertions etc. I suppose? Is there anything left to do regarding this PR? 😄

Would you be willing to pick up the suggestions from @jnyrup? Then we would have a single PR that adds this new API in a consistent API.

For consistency with the ActionAssertions. Mirrors the implementation for Actions.

dennisdoomen · 2018-12-29T15:25:52Z

@jnyrup aren't your comments already addressed?

Src/FluentAssertions/Specialized/AsyncFunctionAssertions.cs

This should make sure that exceptions will reference the correct part of the code.

Src/FluentAssertions/Specialized/AsyncFunctionAssertions.cs

jnyrup · 2018-12-30T21:07:49Z

@frederik-h regarding the two failing tests, you might want to relax the time limits.
Timing tests easily fluctuate on slower machines, such as build servers.

jnyrup · 2018-12-30T22:22:08Z

Regarding the documentation, a description of NotThrowAfter could be added just after
act.Should().NotThrow<InvalidOperationException>();

and just add an example to the block of async examples.

This should help to avoid test failure on slow systems

Remove async/await which are not necessary here

The wait times of two tests were too low. This caused the tests to fail on a slow system.

Fixup whitespace

dennisdoomen · 2019-01-08T20:08:52Z

So? Is this one ready to be merged?

jnyrup · 2019-01-08T20:25:15Z

Should NotThrowAfter for Func<T> return the result for further assertions, just as NotThrow?

Previously, the return type of NotThrowAfter for Func<T> was void. Now we return the result of the executed func to allow for further assertions on that result.

jnyrup · 2019-01-09T19:30:17Z

Tests/Shared.Specs/FunctionExceptionAssertionSpecs.cs

+            //-----------------------------------------------------------------------------------------------------------
+            // Assert
+            //-----------------------------------------------------------------------------------------------------------
+            act.Subject.Should().Be(42);


Just to double check.
Could this be written as

throwShorterThanWaitTime.Should().NotThrowAfter(waitTime, pollInterval) .Which.Should().Be(42);

jnyrup · 2019-01-11T18:46:34Z

@frederik-h Thanks for implementing this feature and contributing to Fluent Assertions.
It takes some endurance as a contributor to go through three months of reviewing (and silence), but I'm really satisfied with the merged result.

dennisdoomen · 2019-01-11T18:57:26Z

Completely agree with @jnyrup here. Because of the maturity of FA, it becomes much more difficult to get new functionality in. So thanks for that.

@jnyrup I think this deserved a release, right?

jnyrup · 2019-01-11T19:25:07Z

@dennisdoomen Push the button.

frederik-h · 2019-01-12T18:31:25Z

Thank you for the constructive review!

frederik-h added 2 commits October 8, 2018 20:31

Add wait time to NotThrow

0e8d5f8

This extends the NotThrow Action assertion by an overload which allows to specify a wait time. This asserts that the Action should stop throwing exceptions after the wait time has passed.

Rename NotThrow overload with wait time

b7a41f8

The overload of NotThrow with waiting time is renamed to NotThrowAfter to emphasize its purpose.

jnyrup requested changes Oct 9, 2018

View reviewed changes

dennisdoomen requested changes Oct 11, 2018

View reviewed changes

Changes requested in review of NotThrowAfter

2da5e3b

* Change waitTime and pollInterval to TimeSpan * Use Thread.Sleep instead of Task.Delay * Add better exception messages if waitTime or pollInterval are out of range * Add tests for invalid argument exceptions * Simplify tests

jnyrup reviewed Oct 12, 2018

View reviewed changes

Minor variable naming and exception message change

d83dd87

* Change local variable names to start with a lower case letter in tests of the NotThrowAfter assertion * Change exception message if NotThrowAfter

Use portable method for sleeping in NotThrowAfter

b46d678

The implementation of NotThrowAfter uses Thread.Sleep. This is not supported in older .net standard versions. Hence, Thread.Sleep is replaced by a ManualResetEvent which also allows to wait for a specified time.

jnyrup reviewed Nov 7, 2018

View reviewed changes

frederik-h added 4 commits November 9, 2018 20:07

Fix test name

b6a22e1

Test "NotThrowAfter_when_subject_is_async_it_should_not_throw" asserts that NotThrowAfter should throw and hence it is renamed accordingly.

Fix exception message in NotThrowAfter

5d80597

The messages of the exceptions that are raised if the TimeSpan parameters of NotThrowAfter are negative claim that the parameters should be "positive". Changed to "non-negative". Further change: * Minor formatting change

Disable NotThrowAfter for netstandard < 2.0

fe90171

Older versions of netstandard don't support Thread.Sleep() which is required by NotThrowAfter.

frederik-h force-pushed the NotThrow-Wait branch from 6bafda9 to fe90171 Compare November 19, 2018 19:03

Merge remote-tracking branch 'upstream/master' into NotThrow-Wait

a8bdaf9

Merge latest changes from upstream

dennisdoomen approved these changes Nov 22, 2018

View reviewed changes

Src/FluentAssertions/Specialized/ActionAssertions.cs Outdated Show resolved Hide resolved

Src/FluentAssertions/Specialized/ActionAssertions.cs Show resolved Hide resolved

Tests/Shared.Specs/ExceptionAssertionSpecs.cs Outdated Show resolved Hide resolved

Minor coding style changes

a9aa045

* Add braces to if * Invert #if condition

Minor code formatting fixes

670d278

Add braces to single line ifs.

frederik-h added 2 commits December 20, 2018 20:18

Add NotThrowAfter to FunctionAssertions

f1525e5

For consistency with the ActionAssertions. Mirrors the implementation for Actions.

Add NotThrowAfter for Func<Task>

f22fcdb

frederik-h force-pushed the NotThrow-Wait branch from c972cf7 to f22fcdb Compare December 20, 2018 19:36

jnyrup reviewed Dec 29, 2018

View reviewed changes

Src/FluentAssertions/Specialized/AsyncFunctionAssertions.cs Outdated Show resolved Hide resolved

Wrap assertion code of NotThrowAfter in local function

bb1e1a0

This should make sure that exceptions will reference the correct part of the code.

jnyrup requested changes Dec 30, 2018

View reviewed changes

Src/FluentAssertions/Specialized/AsyncFunctionAssertions.cs Outdated Show resolved Hide resolved

Src/FluentAssertions/Specialized/AsyncFunctionAssertions.cs Outdated Show resolved Hide resolved

frederik-h and others added 6 commits January 1, 2019 21:10

Increase wait times in NotThrowAfter tests

36a850c

This should help to avoid test failure on slow systems

Change way NowThrowAfterAsync calls the assertion task

e3004c2

Remove async/await which are not necessary here

Add documentation for NotThrowAfter

cfc76ca

Increase wait time in NotThrowAfter-tests

40258c0

The wait times of two tests were too low. This caused the tests to fail on a slow system.

Fixup whitespace

7da866c

Merge pull request #1 from jnyrup/NotBeInDescendingOrder

ccfeeca

Fixup whitespace

jnyrup approved these changes Jan 8, 2019

View reviewed changes

Change NotThrowAfter for Func<T> to return result

49ec86f

Previously, the return type of NotThrowAfter for Func<T> was void. Now we return the result of the executed func to allow for further assertions on that result.

jnyrup reviewed Jan 9, 2019

View reviewed changes

jnyrup merged commit 6cf1d34 into fluentassertions:master Jan 11, 2019

Add Should().NotThrowAfter assertion for actions #942

Add Should().NotThrowAfter assertion for actions #942

Conversation

frederik-h commented Oct 8, 2018 • edited

IMPORTANT

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederik-h Oct 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederik-h Oct 11, 2018 • edited by jnyrup

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnyrup commented Oct 12, 2018

jnyrup commented Oct 13, 2018 • edited

frederik-h commented Oct 26, 2018

jnyrup commented Oct 26, 2018

frederik-h commented Oct 29, 2018

jnyrup commented Nov 7, 2018

dennisdoomen commented Nov 7, 2018

jnyrup commented Nov 7, 2018

dennisdoomen commented Nov 7, 2018

frederik-h commented Nov 8, 2018 • edited

frederik-h commented Nov 19, 2018

frederik-h commented Nov 20, 2018

dennisdoomen commented Nov 20, 2018

dennisdoomen left a comment

Choose a reason for hiding this comment

jnyrup commented Nov 22, 2018

dennisdoomen commented Nov 22, 2018

frederik-h commented Dec 4, 2018

dennisdoomen commented Dec 4, 2018

dennisdoomen commented Dec 29, 2018

jnyrup commented Dec 30, 2018

jnyrup commented Dec 30, 2018

dennisdoomen commented Jan 8, 2019

jnyrup commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnyrup commented Jan 11, 2019

dennisdoomen commented Jan 11, 2019

jnyrup commented Jan 11, 2019

frederik-h commented Jan 12, 2019

frederik-h commented Oct 8, 2018 •

edited

frederik-h Oct 11, 2018 •

edited

frederik-h Oct 11, 2018 •

edited by jnyrup

jnyrup commented Oct 13, 2018 •

edited

frederik-h commented Nov 8, 2018 •

edited