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

Add Should().NotThrowAfter assertion for actions #942

Merged
merged 23 commits into from Jan 11, 2019

Conversation

@frederik-h
Copy link
Contributor

frederik-h commented Oct 8, 2018

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

  • 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.
frederik-h added 2 commits Sep 18, 2018
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.
/// 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)

This comment has been minimized.

Copy link
@jnyrup

jnyrup Oct 9, 2018

Collaborator

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

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Oct 11, 2018

Member

🔧 Maybe NotThrowAnymoreAfter

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Oct 11, 2018

Member

🤔 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?

This comment has been minimized.

Copy link
@frederik-h

frederik-h Oct 11, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@frederik-h

frederik-h Oct 11, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Oct 11, 2018

Member

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

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Oct 11, 2018

Member

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().

This comment has been minimized.

Copy link
@frederik-h

frederik-h Oct 11, 2018

Author Contributor

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 Outdated Show resolved Hide resolved
Tests/Shared.Specs/ExceptionAssertionSpecs.cs Outdated Show resolved Hide resolved
Tests/Shared.Specs/ExceptionAssertionSpecs.cs Outdated Show resolved Hide resolved
Tests/Shared.Specs/ExceptionAssertionSpecs.cs Outdated Show resolved Hide resolved
Tests/Shared.Specs/ExceptionAssertionSpecs.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Specialized/ActionAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Specialized/ActionAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Specialized/ActionAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Specialized/ActionAssertions.cs Outdated Show resolved Hide resolved
Tests/Shared.Specs/ExceptionAssertionSpecs.cs Outdated Show resolved Hide resolved
/// 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)

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Oct 11, 2018

Member

🔧 Maybe NotThrowAnymoreAfter

/// 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)

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Oct 11, 2018

Member

🤔 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
@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 12, 2018

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

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

frederik-h commented Oct 26, 2018

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

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 26, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

frederik-h commented Oct 29, 2018

@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?

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 7, 2018

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

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 7, 2018

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

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Contributor Author

frederik-h commented Nov 8, 2018

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.

frederik-h added 4 commits Nov 9, 2018
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.
@frederik-h frederik-h force-pushed the frederik-h:NotThrow-Wait branch from 6bafda9 to fe90171 Nov 19, 2018
Merge latest changes from upstream
@frederik-h

This comment has been minimized.

Copy link
Contributor Author

frederik-h commented Nov 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

frederik-h commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 20, 2018

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

No, that one is unstable.

Copy link
Member

dennisdoomen left a comment

Only minor comments. Feel free to reject them.

* Add braces to if
* Invert #if condition
@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 22, 2018

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

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 22, 2018

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

This comment has been minimized.

Copy link
Contributor Author

frederik-h commented Dec 4, 2018

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

This comment has been minimized.

Copy link
Member

dennisdoomen commented Dec 4, 2018

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.

frederik-h added 2 commits Dec 7, 2018
For consistency with the ActionAssertions.
Mirrors the implementation for Actions.
@frederik-h frederik-h force-pushed the frederik-h:NotThrow-Wait branch from c972cf7 to f22fcdb Dec 20, 2018
@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Dec 29, 2018

@jnyrup aren't your comments already addressed?

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

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Dec 30, 2018

@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

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Dec 30, 2018

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.

frederik-h and others added 6 commits Jan 1, 2019
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
@jnyrup
jnyrup approved these changes Jan 8, 2019
@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Jan 8, 2019

So? Is this one ready to be merged?

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Jan 8, 2019

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.
//-----------------------------------------------------------------------------------------------------------
// Assert
//-----------------------------------------------------------------------------------------------------------
act.Subject.Should().Be(42);

This comment has been minimized.

Copy link
@jnyrup

jnyrup Jan 9, 2019

Collaborator

Just to double check.
Could this be written as

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

This comment has been minimized.

Copy link
@frederik-h

frederik-h Jan 10, 2019

Author Contributor

Yes!

@jnyrup jnyrup merged commit 6cf1d34 into fluentassertions:master Jan 11, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Jan 11, 2019

@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

This comment has been minimized.

Copy link
Member

dennisdoomen commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Jan 11, 2019

@dennisdoomen Push the button.

@frederik-h

This comment has been minimized.

Copy link
Contributor Author

frederik-h commented Jan 12, 2019

Thank you for the constructive review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.