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

Support assertions for methods returning ValueTask and ValueTask<T> #1158

Merged
merged 3 commits into from
Dec 11, 2019
Merged

Support assertions for methods returning ValueTask and ValueTask<T> #1158

merged 3 commits into from
Dec 11, 2019

Conversation

danielcweber
Copy link
Contributor

@danielcweber danielcweber commented Oct 7, 2019

Explicitly take netcoreapp2.1 and netstandard2.1 into account in FluentAssertions.csproj.

  • 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 to documentation.md in this pull request so the documentation will appear on the website.

@jnyrup
Copy link
Member

jnyrup commented Oct 9, 2019

If we start to support ValueTask<T>, we should also add support for the non-generic ValueTask.
And probably add support for all overloads that currently takes Task/Task<T>.

We should also make sure that we follow the guidelines about usage of ValueTask

The following operations should never be performed on a ValueTask instance:

  • Awaiting the instance multiple times.
  • Calling AsTask multiple times.
  • Using .Result or .GetAwaiter().GetResult() when the operation hasn't yet completed, or using them multiple times.
  • Using more than one of these techniques to consume the instance.

@danielcweber
Copy link
Contributor Author

Non-generic ValueTask is indeed supported in my PR. The "Awaiting" assertions don't change the return types though, and call AsTask just once, so it's safe according to the guidelines you quoted.

@dennisdoomen
Copy link
Member

So what's the status of this PR?

@danielcweber
Copy link
Contributor Author

danielcweber commented Nov 5, 2019

So what's the status of this PR?

Sorry, did I miss something you want me to change? Do you consider this change useful?

@dennisdoomen
Copy link
Member

So what if just add .NET Standard 2.1, and skip .NET Core 2.0? It's a new feature, so I'm fine if it only works for that platform.

I think we should do this one.

@jnyrup are your concerns addressed in a satisfactory way?

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.

https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.valuetask-1?view=netcore-3.0#applies-to

  • ValueTask<T> is available since Net Core 1.0 / .Net Standard 2.1
  • ValueTask is available since Net Core 2.1 / .Net Standard 2.1

Which matches availability of Span<T> (#902 )

With the proposed changes in TargetFrameworks
NetCore30.Specs changes from netcoreapp2.1 to netcoreapp2.1 TargetFramework, but already tested via NetCore20.Specs
leaving netstandard2.1 untested.

Would any be left out, if we only add netcoreapp2.1? (Xamarin, UWP, ???)

The limitation remarks are handled by calling AsTask() a you mention.

Src/FluentAssertions/AssertionExtensions.cs Show resolved Hide resolved
@dennisdoomen
Copy link
Member

With the proposed changes in TargetFrameworks
NetCore30.Specs changes from netcoreapp2.1 to netcoreapp2.1 TargetFramework, but already tested via NetCore20.Specs leaving netstandard2.1 untested.

Would any be left out, if we only add netcoreapp2.1? (Xamarin, UWP, ???)

Not sure if I understand you correctly, but if we switch the NetCore20.csproj to NetCore21.csproj, we should be fine right. We don't have to have to directly test .NET Standard 2.1. That's a compiler concern./

@jnyrup
Copy link
Member

jnyrup commented Nov 7, 2019

If we change NetCore20.Specs to NetCore21.specs, then nothing tests the netcoreapp20 target.

@dennisdoomen
Copy link
Member

dennisdoomen commented Nov 7, 2019

And since that one is no longer supported by Microsoft, I think that is fine.

@danielcweber
Copy link
Contributor Author

Pushed 3 more commits to address the required changes. Not sure I understood your intentions correctly though....

@danielcweber
Copy link
Contributor Author

Apparently Nuke doesn't like it...

@dennisdoomen
Copy link
Member

Apparently Nuke doesn't like it...

It's not Nuke, it's XUnit

@danielcweber
Copy link
Contributor Author

It's not a failed test though....is it ?

@dennisdoomen
Copy link
Member

Try to run the build.ps1 locally.

@danielcweber
Copy link
Contributor Author

danielcweber commented Nov 7, 2019

It's not Nuke, it's XUnit

It was due to the Nuke build script. Fixed by 9182d19.

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.

And since that one is no longer supported by Microsoft, I think that is fine.

We're still supporting net45, even though it's been EOL for almost four years.

@@ -735,6 +989,29 @@ public void When_async_method_of_T_succeeds_and_expected_not_to_throw_particular
action.Should().NotThrow();
}

#if NETCOREAPP2_1 || NETSTANDARD2_1
Copy link
Member

Choose a reason for hiding this comment

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

NETSTANDARD2_1 -> NETCOREAPP3_0 four times in this document

@dennisdoomen
Copy link
Member

We're still supporting net45, even though it's been EOL for almost four years.

We'll drop the explicit .NET Core 2.0 target, but we're still supporting it through our .NET Standard 2.0 target. I might be wrong, but I didn't see anything specifically using .NET Core 2.0 stuff.

@jnyrup
Copy link
Member

jnyrup commented Nov 17, 2019

@dennisdoomen the following does not work with netstandard2_0, but with netcoreapp2_0:

  • EventAssertions
  • EventHandlerFactory
  • EventMonitor
  • EventRecorder
  • IMonitor
  • Monitor<T>(this T eventSource, Func<DateTime> utcNow = null)

@dennisdoomen
Copy link
Member

You've tried? If so, that's annoying. Maybe we should then just target both. We'll clean-up the targets with 6.0 anyway/

@jnyrup
Copy link
Member

jnyrup commented Nov 18, 2019

I just took master and changed the target framework from netcoreapp2_0 to netcoreapp2_1.
Then NetCore20.Specs won't compile as Monitor(this T eventSource, ...) is excluded from the NETSTANDARD2_0 target.
That is ultimately excluded because Netstandard2.0 does not include DynamicMethod or ILGenerator. See #859

@danielcweber
Copy link
Contributor Author

Got a little bit lost on direction here....if you need any additional work in this PR, just @ me. Otherwise I think you know better which targets to support and not support in the future...

@dennisdoomen
Copy link
Member

@jnyrup I propose we keep the changes in the PR and merge it. We'll clean up the targets in 6.0 anyway.

@jnyrup
Copy link
Member

jnyrup commented Nov 23, 2019

@danielcweber Sorry for the confusion about the targets.
Could you re-add the netcoreapp2.0 target, such the changes effectively will be addition of netstandard2.1 and netcoreapp2.1?

Just to be sure, let's have both:

  • NetCore20.Specs
  • NetCore21.Specs

@jnyrup
Copy link
Member

jnyrup commented Nov 26, 2019

Just so I don't forget about it:
We should also update the following docs with the added target frameworks:

  • docs/index.html:29+53
  • docs/_config.yml:22
  • docs/_pages/about.md:42+43+46+92
  • Src/FluentAssertions/FluentAssertions.csproj:16

@danielcweber danielcweber reopened this Nov 26, 2019
@danielcweber
Copy link
Contributor Author

There was some odd "this thing took 300ms too long" test fail. Test went well now. But there's maybe a flaky test around.

@jnyrup
Copy link
Member

jnyrup commented Nov 26, 2019

@danielcweber It's the test that keeps on giving... #1085
When the docs are updated, I'll be happy to punch the merge button.

@dennisdoomen
Copy link
Member

@danielcweber if you update the docs as @jnyrup request, we should be ready to merge it.

@danielcweber
Copy link
Contributor Author

^

@jnyrup
Copy link
Member

jnyrup commented Dec 9, 2019

@danielcweber Could you update the index.html line 53 as well?

@danielcweber
Copy link
Contributor Author

Must have slipped through.

@jnyrup jnyrup merged commit 9b3c5f1 into fluentassertions:master Dec 11, 2019
@jnyrup
Copy link
Member

jnyrup commented Dec 11, 2019

@danielcweber Thanks for contributing this!
... and for hanging in there while we made up our minds.

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

Successfully merging this pull request may close these issues.

3 participants