-
Notifications
You must be signed in to change notification settings - Fork 157
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
Refactor async tests #2813
base: main
Are you sure you want to change the base?
Refactor async tests #2813
Conversation
2f67b21
to
a7a689a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff - I like this a lot better than the current approach.
Tests/Realm.Tests/Helpers/RequiresSynchronizationContextAttribute.cs
Outdated
Show resolved
Hide resolved
Tests/Realm.Tests/Helpers/RequiresSynchronizationContextAttribute.cs
Outdated
Show resolved
Hide resolved
a7a689a
to
650e20f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite cool!
* and run the posted work so we don't install a SynchronizationContext for them. | ||
*/ | ||
|
||
if (SynchronizationContext.Current is SynchronizationContext current) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't use the synchronization context that's already there if it exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one. Big fan of simplifications! 👍
8c4a471
to
9ad2be5
Compare
} | ||
|
||
var @delegate = new EventHandler<ErrorEventArgs>(ErrorHandler); | ||
Realms.Sync.Session.Error += @delegate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed already, for this and all other usage we'll need to re-adjust this once the client reset API PR gets merged as the Session.Error
is deprecated from there on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good work. It's gonna make our tests so much prettier in style and faster to read and write.
Co-authored-by: Nikola Irinchev <[email protected]>
e0d0fdd
to
2f6e88b
Compare
Pull Request Test Coverage Report for Build 2016072633
💛 - Coveralls |
4287415
to
6ca7ba9
Compare
This is a (noisy) refactor of our unit tests that extracts the logic behind
TestHelpers.RunAsyncTest
andSyncTestHelpers.RunBaasTestAsync
into two custom NUnit attributes -[RequiresSynchronizationContext]
and[RequiresBaas]
. This allows us to switch to using awaitable test methods instead of wrapping async code inTestHelpers.RunAsyncTest()
calls.I originally intended to just replace
SyncTestHelpers.RunBaasTestAsync
calls with the[RequiresBaas]
attribute and removeTestHelpers.RunAsyncTest
in favor of making the test methods async themselves, but it turned out that NUnit doesn't automatically install a SynchronizationContext and the only way to tell it to do so is to mark your tests as needing a single-threaded apartment, which also causes NUnit to try to run tests inside a STA thread which .NET only supports on Windows. As a workaround I ended up creating[RequiresSynchronizationContext]
, made it inheritable, and slapped it on our base fixture classRealmTest
, so while it exists, there shouldn't be a need to use it in test code if the test class inherits fromRealmTest
.For all intents and purposes now it's enough for a test method to return
Task
(or another awaitable type) to be considered async, and to decorate any baas integration tests with[RequiresBaas]
.