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

Jump to bottom

Added .NET Standard 1.3 and 1.6 support for NotThrowAfter #1050

Merged
merged 2 commits into from May 27, 2019
Merged

Added .NET Standard 1.3 and 1.6 support for NotThrowAfter #1050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3a996c
Added .NET Standard 1.3 and 1.6 support for NotThrowAfter
davidomid May 26, 2019
0831cb8
Merge branch 'master' of https://github.com/fluentassertions/fluentas…
davidomid May 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions Src/FluentAssertions/Specialized/ActionAssertions.cs
View file
Open in desktop
Expand Up @@ -4,9 +4,7 @@
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
#if !NETSTANDARD1_3 && !NETSTANDARD1_6
using System.Threading;
#endif
using System.Threading.Tasks;
using FluentAssertions.Common;
using FluentAssertions.Execution;
using FluentAssertions.Primitives;
Expand Down Expand Up @@ -116,7 +114,6 @@ public void NotThrow(string because = "", params object[] becauseArgs)
}
}

#if !NETSTANDARD1_3 && !NETSTANDARD1_6
/// <summary>
/// Asserts that the current <see cref="Action"/> stops throwing any exception
/// after a specified amount of time.
Expand Down Expand Up @@ -167,14 +164,13 @@ public void NotThrowAfter(TimeSpan waitTime, TimeSpan pollInterval, string becau
}

invocationEndTime = watch.Elapsed;
Thread.Sleep(pollInterval);
Task.Delay(pollInterval).GetAwaiter().GetResult();
Copy link
Member

Choose a reason for hiding this comment

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

How about copying the behavior from nunit to get the best possible from all worlds?

    internal static void BlockingDelay(int milliseconds)
    {
#if !PARALLEL
        System.Threading.Tasks.Task.Delay(milliseconds).GetAwaiter().GetResult();
#else
        Thread.Sleep(milliseconds);
#endif
    }

where PARALLEL is defined as

<DefineConstants Condition="'$(TargetFramework)' != 'netstandard1.4'
                            and '$(TargetFramework)' != 'netcoreapp1.1'">$(DefineConstants);PARALLEL;SERIALIZATION</DefineConstants>

we could use the !(NETSTANDARD1_3 || NETSTANDARD1_6) instead of PARALLEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find this interesting, because as far as I'm aware there is no benefit to using one or the other, considering both are blocking calls anyway and from my understanding offer similar performance. For me it makes sense to just use Task.Delay.

Do you know of a particular reason to favour Thread.Sleep in some situations instead? I don't doubt that this was done for a good reason for NUnit, I just want to make sure I understand the decision.

Thanks very much :)

Copy link
Member

Choose a reason for hiding this comment

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

Wondering the same myself...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dennisdoomen @jnyrup

I've looked extensively into this and can't find any reason to use Thread.Sleep instead. Unless I'm missing something, are we happy to say just using Task.Delay is fine?

Copy link
Member

Choose a reason for hiding this comment

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

I'll be the first to admit that I'm no async expert.
I don't know if there are any differences between the two approaches, which are relevant to us.
E.g. whether Task.Delay().GetAwaiter().GetResult() is sync-over-async and could cause a deadlock.

I found three related SO posts, but no indication of whether one should be avoid at all costs.
https://stackoverflow.com/questions/53648014/what-is-going-on-with-task-delay-wait
https://stackoverflow.com/questions/34052381/thread-sleep2500-vs-task-delay2500-wait
https://stackoverflow.com/questions/20082221/when-to-use-task-delay-when-to-use-thread-sleep

To summarize, I'm fine with Task.Delay.

Copy link
Contributor Author

@davidomid davidomid May 27, 2019 •
edited

Choose a reason for hiding this comment

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

That top link is actually really interesting. I'll try to benchmark that at some point and figure out if it's a major issue.

I guess in general we should make sure whenever we're using Thread.Sleep or Task.Delay, we treat it less like "I'm guaranteed to wait exactly x milliseconds" and more like "I'm guaranteed to wait at least x milliseconds".

For now though, are we happy to resolve this comment and continue using Task.Delay for now?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that.

}

Execute.Assertion
.BecauseOf(because, becauseArgs)
.FailWith("Did not expect any exceptions after {0}{reason}, but found {1}.", waitTime, exception);
}
#endif

private Exception InvokeSubjectWithInterception()
{
Expand Down
8 changes: 2 additions & 6 deletions Src/FluentAssertions/Specialized/FunctionAssertions.cs
View file
Open in desktop
@@ -1,9 +1,7 @@
using System;
using System.Diagnostics;
using System.Linq;
#if !NETSTANDARD1_3 && !NETSTANDARD1_6
using System.Threading;
#endif
using System.Threading.Tasks;
using FluentAssertions.Execution;
using FluentAssertions.Primitives;

Expand Down Expand Up @@ -128,7 +126,6 @@ private static void NotThrow(Exception exception, string because, object[] becau
}
}

#if !NETSTANDARD1_3 && !NETSTANDARD1_6
/// <summary>
/// Asserts that the current <see cref="Func{T}"/> stops throwing any exception
/// after a specified amount of time.
Expand Down Expand Up @@ -181,7 +178,7 @@ private static void NotThrow(Exception exception, string because, object[] becau
}

invocationEndTime = watch.Elapsed;
Thread.Sleep(pollInterval);
Task.Delay(pollInterval).GetAwaiter().GetResult();
}

Execute.Assertion
Expand All @@ -190,7 +187,6 @@ private static void NotThrow(Exception exception, string because, object[] becau

return new AndWhichConstraint<FunctionAssertions<T>, T>(this, default(T));
}
#endif

private ExceptionAssertions<TException> Throw<TException>(Exception exception, string because, object[] becauseArgs)
where TException : Exception
Expand Down
2 changes: 0 additions & 2 deletions Tests/Shared.Specs/ExceptionAssertionSpecs.cs
View file
Open in desktop
Expand Up @@ -979,7 +979,6 @@ public void When_no_exception_should_be_thrown_and_none_was_it_should_not_throw(
foo.Invoking(f => f.Do()).Should().NotThrow();
}

#if !NETSTANDARD1_3 && !NETSTANDARD1_6 && !NETCOREAPP1_1
#pragma warning disable CS1998
[Fact]
public void When_subject_is_async_it_should_throw()
Expand Down Expand Up @@ -1108,7 +1107,6 @@ public void When_no_exception_should_be_thrown_after_wait_time_and_none_was_it_s
//-----------------------------------------------------------------------------------------------------------
act.Should().NotThrow();
}
#endif // NotThrowAfter tests

}

Expand Down
2 changes: 0 additions & 2 deletions Tests/Shared.Specs/FunctionExceptionAssertionSpecs.cs
View file
Open in desktop
Expand Up @@ -462,7 +462,6 @@ public void When_an_assertion_fails_on_NotThrow_succeeding_message_should_be_inc
#endregion

#region NotThrowAfter
#if !NETSTANDARD1_3 && !NETSTANDARD1_6 && !NETCOREAPP1_1
[Fact]
public void When_wait_time_is_negative_it_should_throw()
{
Expand Down Expand Up @@ -636,7 +635,6 @@ public void When_an_assertion_fails_on_NotThrowAfter_succeeding_message_should_b
"*to be <null>*"
);
}
#endif // NotThrowAfter tests
#endregion
#region NotThrow<T>
[Fact]
Expand Down