[monitorlib] Retrieve interactions from mock uss #270

punamverma · 2023-10-19T06:17:48Z

USSQualifier can request interuss interactions from mock_uss and add it test steps report.

Mock_uss client gets the interuss interaction.
scenarios provides a method to record the interuss interactions in a test step report.
Added basic methods for test steps to check for GET and POST interuss interactions.
These will be used in uss_qualifier tests.

BenjaminPelletier · 2023-10-19T06:24:19Z

When submitting PRs in the future, please follow the general contributing procedure -- specifically, we'd like to have PRs as drafts until ready to review (meaning CI has passed, or the contributor cannot resolve the CI issue).

BenjaminPelletier

The get_interactions method for the mock_uss client looks good. I think we need scenario documentation before we get to scenario implementation in test_steps.py though. I'm also not clear on why Interactions should be called out separately in FailedChecks given existing Query capture.

BenjaminPelletier · 2023-10-19T06:27:03Z

monitoring/uss_qualifier/reports/report.py

@@ -88,6 +90,9 @@ class TestStepReport(ImplicitDict):
    passed_checks: List[PassedCheck]
    """The checks which successfully passed in this test step"""

+    interuss_interactions: Optional[List[Interaction]]


There should be no need to list the interactions here as the FailedCheck should refer to the Query uss_qualifier used to obtain the Interactions from mock_uss which contains the full Interaction information.

There are a few cases where the uss_qualifier query from mock_uss is not sufficient.

We would not be able to tell if injection by SUT failed due a GET call to mock_uss that. gave invalid data, wrong authorization or, message signing validation, by looking at the query from qualifier only. For data validation tests we want to make sure that it was invalid data that SUT received.

We would need to check interactions, to capture POST by SUT to mock_uss, and validate it.
Hence I think interactions should be provided in the test steps which have test cases depending on these interactions.

The query I'm talking about is when uss_qualifier asks mock_uss for its interactions; the response body for that query is this. We should already be logging that query, and therefore logging the response body, and associating that query with any failed checks. Therefore, List[Interaction] is already in the report and including it in the failed check would be duplicative.

Yes, that's the one I am using to get the interaction logs from mock_uss and analyze them in uss_qualifier for the GET or POST that we are expecting.

Ok, you mean those queries are being recorded. I didn't see the query recorded. In fact I queried and used the response to filter out the required interactions.

uss_qualifier is responsible for reporting what it's doing. A test scenario implementation should record_query whenever uss_qualifier interacts with an external system, and then it should usually reference that query in any checks whose failure was determined based on that query (though we're still working on doing this consistently). Query events have the world icon in sequence view (example).

Removed interuss_interactions.

BenjaminPelletier · 2023-10-19T06:36:31Z

monitoring/uss_qualifier/scenarios/astm/utm/interuss/test_steps.py

+    if mock_uss is not None:
+        interactions = _get_interuss_interactions(scenario, mock_uss, st, test_step)
+        logger.debug(f"Checking for Post to {posted_to_url}")
+        with scenario.check("Expect Notification sent") as check:


There is no documentation supporting this check yet -- let's follow the test scenarios development guide and build from requirements -> implementation plan -> scenario documentation -> scenario implementation. This is already the implementation and it's hard to evaluate without knowing the context in which it will be used via the foundation of the earlier development steps.

As noted in the implementation plan, no plan is needed beforehand when the goal can be advanced with small PRs (this PR is small) that don't have unimplemented dependencies (this PR implicitly depends on scenario documentation that isn't written).

I wanted to provide a context for the changes that I have in this PR. And, thought that these methods can provide the context. But, I do understand that they don't have the documentation with it. Do you want me to add the documentation in this PR? Or should I remove this file and add it to another PR with documentation? I can do it as you think will be better.

I would almost always suggest erring on the side of small PRs with established context. I think the fact that both the API/endpoint for getting interactions and the mock_uss client already exist provide excellent context for get_interactions. The sequence of steps in the test scenarios development guide is designed to ensure sufficient context at each point -- I would suggest waiting to propose this file until the context for the test scenario implementation has been established by completing everything applicable on the requirements -> implementation plan -> scenario documentation -> scenario implementation chain prior to scenario implementation.

Sure, I ll remove this file from the current PR, and add it in later PR.

punamverma · 2023-10-19T20:42:30Z

When submitting PRs in the future, please follow the general contributing procedure -- specifically, we'd like to have PRs as drafts until ready to review (meaning CI has passed, or the contributor cannot resolve the CI issue).

I have changed this PR to draft.
However, I was curious about the process. Should we first create a regular PR, make sure it passes CI, and then convert it to draft? Or should we first create draft PR, and then when all reviewed , then make it a regular PR to run CI for the first time? Can CI be run on draft PR?

BenjaminPelletier · 2023-10-19T21:19:07Z

When submitting PRs in the future, please follow the general contributing procedure -- specifically, we'd like to have PRs as drafts until ready to review (meaning CI has passed, or the contributor cannot resolve the CI issue).

I have changed this PR to draft. However, I was curious about the process. Should we first create a regular PR, make sure it passes CI, and then convert it to draft? Or should we first create draft PR, and then when all reviewed , then make it a regular PR to run CI for the first time? Can CI be run on draft PR?

The pull request should be created as a draft and then marked as ready for review when the CI passes or there is a problem with the CI where reviewer help is needed.

punamverma · 2023-10-24T06:14:17Z

@BenjaminPelletier I have removed the test_steps.py file and the interuss_interactions from step report. Could you please check if PR looks good now. Thanks

BenjaminPelletier · 2023-10-24T15:24:54Z

@BenjaminPelletier I have removed the test_steps.py file and the interuss_interactions from step report. Could you please check if PR looks good now. Thanks

Sure -- you can also mark the PR as ready for review to trigger a next look from reviewers ("Ready for review" button near the bottom of this page).

It looks like this branch needs to be sync'd with the main branch ("This branch is out-of-date with the base branch" near the bottom of this page). I recommend merging main into this branch (that's what I do) -- this preserves all of your edit history so that if the merge breaks something, you can always go back to the commit you know works. Rebasing is fine also, but that rewrites history and so therefore you can't necessarily go back to a known-working state. We always squash PRs when merging which means there's no need to try and maintain a clean edit history on a PR branch (the most common reason to rebase rather than merge) -- "messy" merge commits are totally fine.

punamverma · 2023-10-24T16:03:15Z

Ready for review.

BenjaminPelletier · 2023-10-24T17:22:08Z

monitoring/uss_qualifier/reports/report.py

@@ -22,6 +22,8 @@
 from monitoring.uss_qualifier.scenarios.definitions import TestScenarioTypeName
 from monitoring.uss_qualifier.suites.definitions import TestSuiteActionDeclaration

+from monitoring.mock_uss.interaction_logging.interactions import Interaction


This import is no longer needed

* Recording interuss interactions from mock_uss * Fixed format * Fixing format * Removing import of non-exisitng module * Removing a file that should go in with scenario PR * Removing interaction recording in test steps as per PR review 5a685db

) * Make skipped actions first-class actions in report * Fix tested_requirements * Fix lingering issue from #270

) * Make skipped actions first-class actions in report * Fix tested_requirements * Fix lingering issue from #270 5807899

…nteruss#278) * Make skipped actions first-class actions in report * Fix tested_requirements * Fix lingering issue from interuss#270 5807899

punamverma added 2 commits October 18, 2023 16:42

Recording interuss interactions from mock_uss

87903f9

Fixed format

6b5a2e7

Fixing format

d89bc05

BenjaminPelletier reviewed Oct 19, 2023

View reviewed changes

Removing import of non-exisitng module

b59a065

punamverma marked this pull request as draft October 19, 2023 19:10

punamverma added 3 commits October 19, 2023 15:11

Removing a file that should go in with scenario PR

9683447

Removing interaction recording in test steps as per PR review

22b5c60

Merge branch 'main' into record_interactions_from_mock_uss

45527b6

Merge branch 'main' into record_interactions_from_mock_uss

b6efbe5

punamverma marked this pull request as ready for review October 24, 2023 15:51

BenjaminPelletier approved these changes Oct 24, 2023

View reviewed changes

BenjaminPelletier changed the title ~~Record interactions from mock uss~~ [monitorlib] Retrieve interactions from mock uss Oct 24, 2023

BenjaminPelletier merged commit 5a685db into interuss:main Oct 24, 2023
8 checks passed

BenjaminPelletier added a commit to BenjaminPelletier/monitoring that referenced this pull request Oct 24, 2023

Fix lingering issue from interuss#270

185e905

BenjaminPelletier added a commit that referenced this pull request Oct 24, 2023

[uss_qualifier] Make skipped actions first-class actions in report (#278

5807899

) * Make skipped actions first-class actions in report * Fix tested_requirements * Fix lingering issue from #270

github-actions bot added a commit that referenced this pull request Oct 24, 2023

[uss_qualifier] Make skipped actions first-class actions in report (#278

0a336c6

) * Make skipped actions first-class actions in report * Fix tested_requirements * Fix lingering issue from #270 5807899

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[monitorlib] Retrieve interactions from mock uss #270

[monitorlib] Retrieve interactions from mock uss #270

punamverma commented Oct 19, 2023 •

edited

Loading

BenjaminPelletier commented Oct 19, 2023

BenjaminPelletier left a comment

BenjaminPelletier Oct 19, 2023

punamverma Oct 19, 2023

BenjaminPelletier Oct 19, 2023

punamverma Oct 19, 2023

punamverma Oct 19, 2023

BenjaminPelletier Oct 19, 2023

punamverma Oct 24, 2023

BenjaminPelletier Oct 19, 2023

punamverma Oct 19, 2023

BenjaminPelletier Oct 19, 2023

punamverma Oct 19, 2023

punamverma commented Oct 19, 2023

BenjaminPelletier commented Oct 19, 2023

punamverma commented Oct 24, 2023

BenjaminPelletier commented Oct 24, 2023

punamverma commented Oct 24, 2023

BenjaminPelletier Oct 24, 2023

[monitorlib] Retrieve interactions from mock uss #270

[monitorlib] Retrieve interactions from mock uss #270

Conversation

punamverma commented Oct 19, 2023 • edited Loading

BenjaminPelletier commented Oct 19, 2023

BenjaminPelletier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punamverma commented Oct 19, 2023

BenjaminPelletier commented Oct 19, 2023

punamverma commented Oct 24, 2023

BenjaminPelletier commented Oct 24, 2023

punamverma commented Oct 24, 2023

Choose a reason for hiding this comment

punamverma commented Oct 19, 2023 •

edited

Loading