Make Actors unit testable #576
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rynowak
commented
Feb 1, 2021
| { | ||
| throw new InvalidOperationException( | ||
| "The actor was initialized without an HTTP client, and so cannot interact with timers or reminders. " + | ||
| "This is likely to happen inside a unit test."); |
There was a problem hiding this comment.
As described in the issue, this was already impossible to unit test. Now it just fails with a reasonable error message. I logged #575 to track improving this.
rynowak
commented
Feb 1, 2021
| @@ -22,21 +22,17 @@ public sealed class ActorHost | |||
| /// <param name="jsonSerializerOptions">The <see cref="JsonSerializerOptions"/> to use for actor state persistence and message deserialization.</param> | |||
| /// <param name="loggerFactory">The logger factory.</param> | |||
| /// <param name="proxyFactory">The <see cref="ActorProxyFactory" />.</param> | |||
| /// <param name="daprInteractor">The <see cref="IDaprInteractor" />.</param> | |||
| internal ActorHost( | |||
| public ActorHost( | |||
There was a problem hiding this comment.
Users need to be able to access this for unit testing.
There was a problem hiding this comment.
Thanks! My earlier change broke this.
rynowak
commented
Feb 1, 2021
| var host = new ActorHost(this.ActorTypeInfo, actorId, this.jsonSerializerOptions, this.loggerFactory, this.proxyFactory) | ||
| { | ||
| DaprInteractor = this.daprInteractor, | ||
| StateProvider = new DaprStateProvider(this.daprInteractor, this.jsonSerializerOptions), |
There was a problem hiding this comment.
The fix for #575 will be to do the same thing for reminders as we do for state. The Actor class is using the http interactor directly instead of a higher level abstraction, so it's not mockable.
rynowak
commented
Feb 1, 2021
rynowak
force-pushed
the
rynowak/release-1.0.0/fix-actor-testing
branch
from
457dcc2
to
de0c7cb
Compare
Fixes: #574 Changes ActorHost constructor to be public again. Now the state manager and http interactor are set via properties. This means that code in unit tests won't be able to cover the methods that interact with timers or reminders - however this was already the case. Testing state management is covered by replacing the `IActorStateManager` with a mock. Logged an issue to improve this. Also added validation. If you try to use something that's not fully initialized it will result in an exception with a meaningful message instead of a null reference. Also skips tests that are failing due to #573. We know the cause of why these are failing, and these tests have not been stable since they were introduced. Failing additional CI builds is not giving us new information.
rynowak
force-pushed
the
rynowak/release-1.0.0/fix-actor-testing
branch
from
de0c7cb
to
f5a0807
Compare
Codecov Report
@@ Coverage Diff @@
## release-1.0.0 #576 +/- ##
=================================================
- Coverage 65.85% 61.62% -4.24%
=================================================
Files 120 120
Lines 3957 3987 +30
Branches 436 438 +2
=================================================
- Hits 2606 2457 -149
- Misses 1231 1417 +186
+ Partials 120 113 -7
Continue to review full report at Codecov.
|
|
The negative coverage diff is due to skipping tests that have flaky behavior. Restoring these tests is tracked separately. |
vinayada1
approved these changes
Feb 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Changes ActorHost constructor to be public again. Now the state manager
and http interactor are set via properties. This means that code in unit
tests won't be able to cover the methods that interact with timers or
reminders - however this was already the case. Testing state management
is covered by replacing the
IActorStateManagerwith a mock.Logged an issue to improve this.
Also added validation. If you try to use something that's not fully
initialized it will result in an exception with a meaningful message
instead of a null reference.
Also fixed some sources of indeterminism in our tests. This is still
pretty fragile, but for now the tests will at least be reliable.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Fixes: #573, #574
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: