Mock Actor merge attempt 2 (see desc) #750
Conversation
Signed-off-by: Lorenzo Curcio <[email protected]>
There was a problem hiding this comment.
I'm sorry I haven't gotten to this PR yet, we're all really busy with the 1.15 release.
I noticed you renamed one of the docs files. I think we should revert that change, because this .md is shown directly in the dapr docs: https://docs.dapr.io/developing-applications/sdks/python/python-actor/.
Also, please move the new docs you added in the existing page.
lor1113
force-pushed
the
mock-actors-3
branch
from
b449f3c to
6406b8d
Compare
Signed-off-by: Lorenzo Curcio <[email protected]>
lor1113
force-pushed
the
mock-actors-3
branch
from
6406b8d to
aee0fe0
Compare
|
The file was renamed such that it would be able to have sub-files, like how the main "Extensions" page under the Python SDK documentation is called "index.md" (https://docs.dapr.io/developing-applications/sdks/python/python-sdk-extensions/) with the idea that the mock actor documentation could be confined to a sub-file for brevity. Requested changes have been made regardless. It's now simply appended to the end of the current actor documentation. |
Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Lorenzo Curcio <[email protected]>
lor1113
force-pushed
the
mock-actors-3
branch
from
1402da4 to
9d5e626
Compare
Signed-off-by: Lorenzo Curcio <[email protected]>
|
This should fix the formatting and type issues (with some type:ignores, but I don't think there's a better way without significant work), I also re-ran the tests to make sure they didn't break since I last checked them (worth noting that I do actually get some tests breaking, some random grpc client ones, possibly because I'm just running directly on windows and not using a dev container since it kept breaking for me). All the actor tests + the new ones I added run OK at least. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #750 +/- ##
==========================================
- Coverage 86.63% 84.92% -1.71%
==========================================
Files 84 89 +5
Lines 4473 4975 +502
==========================================
+ Hits 3875 4225 +350
- Misses 598 750 +152 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Looks great @lor1113!
I suggested some text cleanup in the docs (thank you for writing such a detailed guide).
As you had noticed before, there's a bug in the try_add_state method of the state manager, and I already sent a pr here: #756.
Feel free to update your mock to follow the same logic.
Two requests: please add a unit test for the mock state manager and an example for using a mock in the examples directory, that also serves as our suite of integration tests.
Again, great work!
| @@ -69,6 +70,9 @@ def __init__(self, actor: 'Actor'): | |||
| self._type_name = actor.runtime_ctx.actor_type_info.type_name | |||
|
|
|||
| self._default_state_change_tracker: Dict[str, StateMetadata] = {} | |||
| self._mock_state: Dict[str, Any] | |||
There was a problem hiding this comment.
If you don't add this, then because of the type hinting problems mentioned earlier, the _mock_state, _mock_reminders and _mock_timer variables won't be correctly type hinted by the IDE, and will show up as white unknown variables.
With those type hint stubs
Without
Yes the code would still run just fine, but it's not very nice, no?
Type hinting the MockStateManager is of no use, because (as you can see in the image) it thinks the _state_manager variable it is dealing with is an ActorStateManager instead of MockStateManager.
In my view adding these stubs to ActorStateManager is harmless (at least, I cannot think of a scenario where they might cause problems or be undesirable) so I think they're worth having.
There was a problem hiding this comment.
I'm leaning more towards removing the _mock arguments from the main code, and rely on good documentation (which @lor1113 already did) for awareness of the properties' existence. What do you think @berndverst ?
Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
This reverts commit 254ad17. Signed-off-by: Lorenzo Curcio <[email protected]>
Fixing bug in try_add_state as mentioned in PR dapr#756 Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
lor1113
force-pushed
the
mock-actors-3
branch
from
caf96db to
6cebadd
Compare
Whoops missed this Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Lorenzo Curcio <[email protected]>
lor1113
force-pushed
the
mock-actors-3
branch
from
ac6e934 to
33a5d1d
Compare
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Lorenzo Curcio <[email protected]>
didnt see this earlier whoops Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
| **The `_on_activate` method will not be called automatically the way it is when Dapr initializes a new Actor instance. You should call it manually as needed as part of your tests.** | ||
|
|
||
| The `__init__`, `register_timer`, `unregister_timer`, `register_reminder`, `unregister_reminder` methods are all overwritten by the MockActor class that gets applied as a mixin via `create_mock_actor`. If your actor itself overwrites these methods, those modifications will themselves be overwritten and the actor will likely not behave as you expect. |
There was a problem hiding this comment.
Can we suggest a workaround here? For example, creating their own mock actor wrapper, using our mock.
There was a problem hiding this comment.
It's a bit hard to think about how to suggest a workaround here, as by definition if someone is overriding those methods they're already doing something pretty out-of-the-box where it's hard to suggest a generic fix. But I did add in something in the latest commit, see if you approve.
| ### Usage and Limitations | ||
|
|
||
| **The `_on_activate` method will not be called automatically the way it is when Dapr initializes a new Actor instance. You should call it manually as needed as part of your tests.** |
There was a problem hiding this comment.
Is this by design, so users can have more control _on_activate? If yes, please mention that.
| The actor `_runtime_ctx` variable is set to None. All the normal actor methods have been overwritten such as to not call it, but if your code itself interacts directly with `_runtime_ctx`, it will likely break. | ||
|
|
||
| The actor _state_manager is overwritten with an instance of `MockStateManager`. This has all the same methods and functionality of the base `ActorStateManager`, except for using the various `_mock` variables for storing data instead of the `_runtime_ctx`. If your code implements its own custom state manager it will be overwritten and your code will likely break. |
There was a problem hiding this comment.
your code will likely break
This can sound a bit scary to users :) Let's say "tests may fail" instead.
There was another instance of the same phrasing, I think, I'd update that one too.
| @@ -69,6 +70,9 @@ def __init__(self, actor: 'Actor'): | |||
| self._type_name = actor.runtime_ctx.actor_type_info.type_name | |||
|
|
|||
| self._default_state_change_tracker: Dict[str, StateMetadata] = {} | |||
| self._mock_state: Dict[str, Any] | |||
There was a problem hiding this comment.
I'm leaning more towards removing the _mock arguments from the main code, and rely on good documentation (which @lor1113 already did) for awareness of the properties' existence. What do you think @berndverst ?
Co-authored-by: Elena Kolevska <[email protected]> Signed-off-by: Lorenzo Curcio <[email protected]>
Signed-off-by: Lorenzo Curcio <[email protected]>
New merge request made due to manually copying changes into a new branch being seemingly the only way to avoid weird git diff issues.
Description
Added Mock Actor capability.
NOTE: Due to not being able to get dapr-docs running in its own dev container despite repeated attempts (I am simply not a frontend coder so i don't know how to debug all the weird errors it was spitting out), I have not been able to actually view the new documentation within a running instance of the website. I have written it nonetheless using the same templating as the others, as well as basic markdown syntax, but it would be appreciated if someone who has a develop version of the docs website working on their machine could just give it a quick look to make sure it's not somehow broken.
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.
Please reference the issue this PR will close: #737
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: