Enterprise Unit Testing Sample #310
Conversation
|
cc @scgbear , @davidmrdavid for review and merge. |
|
Adding Connor as a review since this also exemplifies how to use EasyAuth w/ Durable |
|
Reason why the CI is failing: I fixed the tests folder in samples, but it looks like pytest is trying to run tests, and it cannot find IsolatedAsyncioTestCase, looks like there is some pytest/unittest compatibility - IsolatedAsyncioTestCase is required to test the async methods in durable, else a co-routine object will be returned that is not worth inspecting. I guess the best thing to do for now is to escape samples folder for CI/CD - or maybe samples can have their own test runner. ImportError while importing test module '/home/vsts/work/1/s/samples/unit_testing/subscription-manager/unit_tests/test_createenvhttpstart_invalidauth.py'. |
There was a problem hiding this comment.
I finished a first pass over this PR. Overall, it looks good and well-structured, but I find it to be a bit hard to follow as there's a lot going on.
To be fully transparent, I think we'll probably have to iterate through this a few times until
(1) I get a full understanding of the scenario
(2) We simplify things a bit
Thanks for your patience with us, and please see my comments below. Thank you!!!
|
|
||
| This also demonstrates usage of: | ||
|
|
||
| - EasyAuth using decoraters |
There was a problem hiding this comment.
tiny nit
| - EasyAuth using decoraters | |
| - EasyAuth using decorators | |
There was a problem hiding this comment.
Sounds good. This is a canonical customer user case so adapting it into a simpler form and keeping the balance between canonical use case vs simplicity is key. Will iterate till it reaches a shape we can agree on
| This project demonstrates a durable workflow that manages a subscription creation long running lifecyle and is adapted from a canonical | ||
| real world example. | ||
| The durable orchestration, will create a subscription, wait for the subscription to be created (through the durable timer) | ||
| and update status of subscription creation in an in-memory status object. |
There was a problem hiding this comment.
Nit: fixing spacing and tiny typo
| This project demonstrates a durable workflow that manages a subscription creation long running lifecyle and is adapted from a canonical | |
| real world example. | |
| The durable orchestration, will create a subscription, wait for the subscription to be created (through the durable timer) | |
| and update status of subscription creation in an in-memory status object. | |
| This project demonstrates a durable workflow that manages a subscription creation long running lifecycle and is adapted from a canonical real world example. | |
| The durable orchestration, will create a subscription, wait for the subscription to be created (through the durable timer) and update status of subscription creation in an in-memory status object. | |
| mock.call_sub_orchestrator.side_effect = sub_orc_mock | ||
| mock.task_all.side_effect = task_all_mock | ||
|
|
||
| # To get generator results do a next. If orchestrator response is a list, then wrap the function call around a list |
There was a problem hiding this comment.
I'm not sure I understand the comment about list-wrapping in here. Why is that necessary?
There was a problem hiding this comment.
We could use next(orchestrator_fn(mock)) as well here, is that more intuitive? using list just makes sure the entire computed result is back for inspection - no big value with list. Let me know if converting it into a next call is more explanatory.
There was a problem hiding this comment.
Ah, I think I understand this now. This is similar to how map() returns a lazily-evaluated list, so it requires a call to list() in order to obtain the materialized result, isn't it?
In this case, I think it would be simpler if we continued using next(), just for consistency :)
| ```python | ||
| def task_all_mock(tasks:list): | ||
| assert len(tasks) == 2 | ||
| return list |
There was a problem hiding this comment.
Why are we returning just list here? Did you mean to return list() or []?
There was a problem hiding this comment.
Yes changed it to [] (this is a mistake due to code churns - sorry!) The task_all never checked back the return type so it probably just returned the list Class and passed.
| The subscription manager uses a custom callback that gets called from another method invoked | ||
| inside of an activity function. The following code demonstrates how to patch these callbacks: |
There was a problem hiding this comment.
The first sentence here is a little complex. Any chance we could simplify it? I also don't think the concept of a subscription manager has been introduced until now, so it would be great to have at least one sentence explaining what that is, or linking to the code :)
There was a problem hiding this comment.
Will do. This was very specific to the customer use case in fact. Actually would want to discuss in general how to do a lambda based call back.
| a_mock.start_new = AsyncMock() | ||
| a_mock().start_new.return_value = instance_id | ||
| a_mock().create_check_status_response.return_value = mock_response | ||
| response = await main(req=mock_request, starter=starter) | ||
| self.assertEqual(403,response.status_code) |
There was a problem hiding this comment.
Just to make sure I understand, this isn't testing the HTTPTrigger itself, this is actually just trying to test the decorator, right? I'm just confused because it seems to be like the return value for create_check_status_response is already being provided
There was a problem hiding this comment.
Yes we are only testing the decorator here and making sure it authorizes and calls the function. So yes, for valid auth we should be removing the canned response. Will discuss further. For the invalid auth, the decorator itself would return a 403 - so we don't really care about the create_check_status_response. but I see why it can be confusing. Will remove this canned response
| """ | ||
| Test class for CreateEnvironmentHTTPStart Durable HTTP Starter Function that kicks off orchestrations | ||
| Mocks the HTTP Request and expected HTTP Response from the Durable HTTP start method and tests | ||
| - invalid route params |
There was a problem hiding this comment.
where is the invalid route param being provided?
There was a problem hiding this comment.
Line 33: clientName is None
There was a problem hiding this comment.
I see. That wasn't entirely clear to me at first :) . I think that abstracting the boilerplate setup away, as suggested in another comment, will help make this clearer
| function_name = 'DurableFunctionsOrchestrator' | ||
| instance_id = 'f86a9f49-ae1c-4c66-a60e-991c4c764fe5' | ||
| productName = 'test_product' | ||
| clientName = 'microsoft' | ||
| environmentName = 'production' | ||
| starter = MagicMock() | ||
|
|
||
| mock_request = func.HttpRequest( | ||
| method='POST', | ||
| body=b'{"sub_product":"test"}', | ||
| url=f'http://localhost:7071/api/orchestrators/product/{productName}/clients/{clientName}/environments/{environmentName}/', | ||
| route_params={ | ||
| 'clientName' : clientName | ||
| }, | ||
| headers={"X-MS-CLIENT-PRINCIPAL": "ICAgICAgICB7CiAgICAgICAgICAgICJhdXRoX3R5cCI6ICJhYWQiLAogICAgICAgICAgICAiY2xhaW1zIjogW3sKICAgICAgICAgICAgICAgICJ0eXAiOiAiaHR0cDovL3NjaGVtYXMueG1sc29hcC5vcmcvd3MvMjAwNS8wNS9pZGVudGl0eS9jbGFpbXMvc3VybmFtZSIsCiAgICAgICAgICAgICAgICAidmFsIjogIlVzZXIiCiAgICAgICAgICAgIH0sIHsKICAgICAgICAgICAgICAgICJ0eXAiOiAiZ3JvdXBzIiwKICAgICAgICAgICAgICAgICJ2YWwiOiAiZWY2ZDJkMWEtNzhlYi00YmIxLTk3YzctYmI4YThlNTA5ZTljIgogICAgICAgICAgICB9LCB7CiAgICAgICAgICAgICAgICAidHlwIjogImdyb3VwcyIsCiAgICAgICAgICAgICAgICAidmFsIjogIjNiMjMxY2UxLTI5YzEtNDQxZS1iZGRiLTAzM2Y5NjQwMTg4OCIKICAgICAgICAgICAgfSwgewogICAgICAgICAgICAgICAgInR5cCI6ICJuYW1lIiwKICAgICAgICAgICAgICAgICJ2YWwiOiAiVGVzdCBVc2VyIgogICAgICAgICAgICB9XSwKICAgICAgICAgICAgIm5hbWVfdHlwIjogImh0dHA6Ly9zY2hlbWFzLnhtbHNvYXAub3JnL3dzLzIwMDUvMDUvaWRlbnRpdHkvY2xhaW1zL25hbWUiLAogICAgICAgICAgICAicm9sZV90eXAiOiAiaHR0cDovL3NjaGVtYXMubWljcm9zb2Z0LmNvbS93cy8yMDA4LzA2L2lkZW50aXR5L2NsYWltcy9yb2xlIgogICAgICAgIH0="} |
There was a problem hiding this comment.
A lot of this setup here seems, at glance, to be repeated across many tests. Is it possible to abstract them away in a common utility? It would help with readability, and to identify what specific change in this setup leads allows the target scenario to be tested.
There was a problem hiding this comment.
Yes agree.
| def call_activity_create_subscription(activityName: str, payload): | ||
| assert activityName == "CreateSubscription" | ||
| mock.call_activity.side_effect = call_activity_mock_status_check_succeeded | ||
|
|
||
| def call_activity_mock_status_check_accepted(activityName: str, payload): | ||
| assert activityName == "StatusCheck" | ||
|
|
||
| def call_activity_mock_status_check_succeeded(activityName: str, payload): | ||
| assert activityName == "StatusCheck" | ||
|
|
||
| def call_activity_mock_status_check_notfound(activityName:str, payload): | ||
| mock.call_activity.side_effect = call_activity_create_subscription | ||
| assert activityName == "StatusCheck" | ||
|
|
||
| def call_activity_mock_status_check_error(activityName:str, payload): | ||
| assert activityName == "StatusCheck" |
There was a problem hiding this comment.
if most of these are do the same, I think it would help simplify things if we just had one method for them all :)
There was a problem hiding this comment.
Yes - I just thought if anyone wants to customize a very specific activity, they could and it helps makes it readable
| # change the call_activity side effect to succeeded call | ||
| mock.call_activity.side_effect = call_activity_mock_status_check_succeeded |
There was a problem hiding this comment.
why is this necessary?
There was a problem hiding this comment.
I guess the confusion is because all the call_activity mocks are the same. While simulating a scenario where sub creation is already underway, where the subscription creation API's would respond with an "Accepted" state - the durable orchestrator will start a timer until this state changes from Accepted -> Succeeded. So I am mocking accordingly. What I can do to make this more meaningful is to return the canned response from each call activity to make them unique.
There was a problem hiding this comment.
Hmm, let's discuss this one during our meeting :)
|
Any particular reason not to use |
|
Hi @binarypunch: we have no particular justification for using one unit testing framework from the other. I think both are similarly popular in the community :) |
| Test class for MgmtGroupSubOrchestrator Durable orchestrator that kicks off an activity function | ||
| Mocks the DurableOrchestrationContext and checks the sequence of sub-orchestration calls | ||
| """ | ||
| class TestMgmtGroupSubOrchestrator(TestCase): |
There was a problem hiding this comment.
Here, have you thought of leveraging what is already in
I also think these classes and methods should be in the main package, and users should be able to import them just with from azure.durable_functions.testing import .... Refactoring what @scgbear and @davidmrdavid did in test_sequential_orchestrator.py for example.
Subscription Manager is a canonical enterprise sample delivered to an enterprise customer. It consists of a durable orchestration workflow to create azure subscriptions and manage their levels and mainly demonstrates how unit tests are written in the current version of durable python.