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

Async overloads for OnMessage and When #148

Merged
merged 1 commit into from Sep 9, 2019

Conversation

@danielmarbach
Copy link
Member

commented Sep 6, 2019

Customers might have tons of those tests but if they are using xunit the GetAwaiter().GetResult() calls will deadlock in certain scenarios. This introduces async variants to allow customers to move to them. This makes it possible to get the tests unstuck from deadlocking and gives them the possibility to gradually then move when the time comes to the new testable overloads

See #135 (comment)

I'm aware that this contradicts the current async method naming guideline but I think for the testing lib for this specific case this is fine

@timbussmann

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

LGTM, however I'm wondering whether we shouldn't actually just remove the sync versions and release a new major version of the package?

@danielmarbach

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

If I recall correctly we made them sync initially to be compatible as much as we can with the previous fluent syntax. Not against it though. It would mean more migration work but it would make them fall into the pit of success with any testing framework. Given that already two users wastes a lot of time with xunit and all other code is async as well it is probably a good strategy to make it async

@danielmarbach

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

I came to the conclusion that we probably can't remove the sync overloads and only provide the async overloads. The problem is that we would expose users to the risk of having tests that pass as green but are actually red due to missing awaits.

When in one major version OnMessage or the When overloads return void or Saga<T> and then in the next major version it returns Task or Task<Saga<T>> having the surrounding method returning void will not trigger a compiler warning for missing awaits. That means the user might have after the upgrade code like this

[Fact]
public void MyTest() {
   .OnMessage();
}

which then becomes a fire & forget method

@timbussmann

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

good point, didn't think about that.

@danielmarbach danielmarbach merged commit 43fcfa9 into master Sep 9, 2019

7 checks passed

Compile Finished TeamCity Build NServiceBus / NServiceBus.Testing / 1. Compile : Running
Details
Inspections Finished TeamCity Build NServiceBus / NServiceBus.Testing / 2. Inspections : Running
Details
Test .NET Core on Linux Finished TeamCity Build NServiceBus / NServiceBus.Testing / 3.3 Test (.NET Core on Linux) : Tests passed: 140
Details
Test .NET Core on Windows Finished TeamCity Build NServiceBus / NServiceBus.Testing / 3.2 Test (.NET Core on Windows) : Tests passed: 140
Details
Test .NET Framework on Windows Finished TeamCity Build NServiceBus / NServiceBus.Testing / 3.1 Test (.NET Framework on Windows) : Tests passed: 140
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@danielmarbach danielmarbach deleted the on-message branch Sep 9, 2019

@danielmarbach danielmarbach added this to the 7.2.0 milestone Sep 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.