Skip to content
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

[uss_qualifier] f3548 - GEN0300 verify interactions with interoperability test instance #397

Merged

Conversation

Shastick
Copy link
Contributor

Extends the f3548 aggregate check to look for queries that interact with the test instance in order to validate requirement n° GEN0300.

If no interaction is found, the check is skipped.

Otherwise, the check is reasonably lax: for each possible interaction with the test endpoint, it is enough to have a single successful query to pass.

def _confirm_test_harness_queries_work(self):
"""
For each different type of call to the interoperability test instance,
we look for at least one successful query.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty lax, but I was wondering it it is reasonable to expect all queries to succeed: if there's a stray network failure/timeout that gets taken care of through a retry it would probably be too stringent to mark the requirement as failed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine, and I do agree that requiring all queries to succeed would be going to far (I don't think we should conclude that a test instance wasn't being provided if 99% of queries to it succeeded). I think our detection of a violation of the requirement to provide a test instance should be pretty robust with this implementation as-is.

@Shastick Shastick force-pushed the gen0300-check-test-harness branch 2 times, most recently from 25c4fc3 to c00d54a Compare December 11, 2023 09:24
@Shastick
Copy link
Contributor Author

Shastick commented Dec 11, 2023

@BenjaminPelletier this approach is slightly different than what I remember discussing ~10 days ago (which I wrote down as "find a check that interacts with the test instance and mark GEN0300 as checked").

As I ran into the QueryType constants that seem to include the interactions with the test instance I opted to add a test case to the aggregate checks at the end.

@Shastick Shastick marked this pull request as ready for review December 11, 2023 09:52
@Shastick Shastick force-pushed the gen0300-check-test-harness branch 2 times, most recently from e336119 to f8586c4 Compare December 12, 2023 14:34
@BenjaminPelletier
Copy link
Member

@BenjaminPelletier this approach is slightly different than what I remember discussing ~10 days ago (which I wrote down as "find a check that interacts with the test instance and mark GEN0300 as checked").

As I ran into the QueryType constants that seem to include the interactions with the test instance I opted to add a test case to the aggregate checks at the end.

This seems like a reasonable approach to me, and probably superior to what I was originally thinking. GEN0300 says the USS needs to provide a test instance and I think this approach effectively identifies when that requirement isn't met with a fairly small amount of implementation.

Copy link
Member

@BenjaminPelletier BenjaminPelletier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Not merging yet because I think main needs to be merged back into this dev branch and make format run to avoid CI complaints when merged to main.

"Interoperability test instance is available", [participant_id]
) as check:
if test_interactions == 0:
# TODO remove once finished with development
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow this; what would be removed at completion of development?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to the test_interactions == 0 condition and the call to record_failed that is guarded by it: given that if the condition is true we returned above already.

(The condition was there to fail noisily while I was building the scenario)

@BenjaminPelletier BenjaminPelletier merged commit 4e814ae into interuss:main Dec 13, 2023
9 checks passed
github-actions bot added a commit that referenced this pull request Dec 13, 2023
…lity test instance (#397)

[uss_qualifier] gen0300 confirm that the test harness works 4e814ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants