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/scenarios/netrid] specify some check severity in test scenario documentation only (contrib #404) #882

Merged
merged 8 commits into from
Jan 16, 2025

Conversation

the-glu
Copy link
Contributor

@the-glu the-glu commented Jan 14, 2025

This PR move some check's severity definition into documentation, contributing to #404.

There are separate commits for easier review. I had issue with spacing and emojis, this is fixed in the last commit.

With shared tests, some checks are not 100% converted (e.g. heavy_traffic_concurent's cleanup).

The removal of severity from code created issues with undocumented checks (at least Recent positions timestamps and Recent positions for aircraft crossing the requested area boundary show only one position before or after crossing).
It does seems that those can be run without documentation, (when called from _assert_evaluate_sp_flight_recent_positions?), and no severity is set when building undocumented check. fd87973 add a default severity as a workaround, but this is something that should probably be discussed.

As an estimation for work left to be done, this reduced usage to severity in scenarios from ~200 to ~170.

Copy link

linux-foundation-easycla bot commented Jan 14, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@the-glu the-glu marked this pull request as draft January 14, 2025 11:14
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.

This looks great. My "nit" comments are primarily placeholders to see whether we should re-evaluate the severity of those check failures (I'm sure you've copied the current values correctly). Please feel free to resolve as you think is appropriate (either accept or reject my suggestions) -- the difference between Medium severity and High severity is whether the test can continue. If later steps depend on the thing we're checking succeeding, then a check failure should be High severity because we can no longer depend on expected behavior when we get to those later steps. Otherwise, Medium severity is preferred.

@mickmis or @barroco FYI this looks good to merge to me after @the-glu addresses the comments

@the-glu
Copy link
Contributor Author

the-glu commented Jan 15, 2025

This looks great. My "nit" comments are primarily placeholders to see whether we should re-evaluate the severity of those check failures (I'm sure you've copied the current values correctly).

Yes, I applied the comments accordingly (in both v22a/v19 in all cases). Some of severity values are coming from the default High severity in handle_query_result and indeed should be lowered :)

@the-glu the-glu marked this pull request as ready for review January 15, 2025 15:28
@the-glu the-glu requested a review from Shastick January 15, 2025 15:28
Copy link
Contributor

@Shastick Shastick left a comment

Choose a reason for hiding this comment

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

Looks good, with some typos

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.

Ready to merge after Shastick's typos IMO

@barroco barroco merged commit 472d64d into interuss:main Jan 16, 2025
20 checks passed
@barroco barroco deleted the move-severity-check branch January 16, 2025 10:56
github-actions bot added a commit that referenced this pull request Jan 16, 2025
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.

4 participants