-
Notifications
You must be signed in to change notification settings - Fork 20
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] Run multiple uss_qualifier configurations #151
[uss_qualifier] Run multiple uss_qualifier configurations #151
Conversation
9bb6108
to
9433472
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this, it is a great improvement.
monitoring/uss_qualifier/README.md
Outdated
@@ -10,7 +10,7 @@ The `uss_qualifier` tool is a synchronous executable built into the `interuss/mo | |||
|
|||
The primary input accepted by uss_qualifier is the "configuration" specified with the `--config` option. This option should be a [reference to a configuration file](configurations/README.md) that the user has constructed or been provided to test the desired system for the desired characteristics. If testing a standard local system (DSS + dummy auth + mock USSs), the user can specify an alternate configuration reference as a single argument to `run_locally.sh` (the default configuration is `configurations.dev.local_test`). | |||
|
|||
When building a custom configuration file, consider starting from [`configurations.dev.self_contained_f3548`](configurations/dev/self_contained_f3548.yaml), as it contains all information necessary to run the test without the usage of sometimes-configuring `$ref`s and `allOf`s. See [configurations documentation](configurations/README.md) for more information. | |||
When building a custom configuration file, consider starting from [`configurations.dev.self_contained_f3548`](configurations/dev/f3548_self_contained.yaml), as it contains all information necessary to run the test without the usage of sometimes-configuring `$ref`s and `allOf`s. See [configurations documentation](configurations/README.md) for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When building a custom configuration file, consider starting from [`configurations.dev.self_contained_f3548`](configurations/dev/f3548_self_contained.yaml), as it contains all information necessary to run the test without the usage of sometimes-configuring `$ref`s and `allOf`s. See [configurations documentation](configurations/README.md) for more information. | |
When building a custom configuration file, consider starting from [`configurations.dev.f3548_self_contained`](configurations/dev/f3548_self_contained.yaml), as it contains all information necessary to run the test without the usage of sometimes-configuring `$ref`s and `allOf`s. See [configurations documentation](configurations/README.md) for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
@@ -200,5 +204,6 @@ def evaluate_operational_status( | |||
) | |||
else: | |||
self._test_scenario.record_note( | |||
f"Unsupported version {self._rid_version}: skipping Operational Status evaluation" | |||
key="skip_reason", | |||
message=f"Unsupported version {self._rid_version}: skipping Operational Status evaluation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning it up. It was actually going to be fixed by #150.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be a race to see where it goes in first :)
"configurations.dev.uspace" \ | ||
) | ||
# TODO: Add configurations.dev.netrid_v19 | ||
# TODO: Add configurations.dev.netrid_v22a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to capture the intention, may I kindly ask you to describe what are the reasons preventing to add those configurations ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: Tracer.
Tracer must (currently) be run in F3411-19 mode OR F3411-22a mode, and the observation area is statically defined on startup. I'm actually working on a PR right now that will make tracer observation areas dynamic so that we won't have this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick test, it is actually breaking on my machine. Investigating the issues.
For reference:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the latest suggestion which should fix the date issue when running locally on macos 13.3.1.
docker_args="-it" | ||
fi | ||
|
||
start_time=$(date +%Y-%m-%dT%H:%M:%S -d '1 second ago') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my mac, it looks like this syntax has been deprecated. The correct one would be:
start_time=$(date +%Y-%m-%dT%H:%M:%S -d '1 second ago') | |
start_time=$(date -v-1S +%Y-%m-%dT%H:%M:%S) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ug, apparently date
is not cross-platform. The old version works in the CI, the new version does not: date: invalid option -- 'v'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon additional research, it looks like date
always rounds down so I think I don't need to subtract that second; I've removed the 1-second offset.
Currently, our CI relies primarily on running one giant "local_test" uss_qualifier configuration which uses many disparate parts of the uss_qualifier codebase. It is unnecessarily difficult to pipe all the resources to the right places, and is hard to isolate just the problem problem area when issues arise. This PR addresses this problem by changing uss_qualifier's run_locally.sh to run many configurations when the specific configuration desired is not specified. local_test is deleted entirely, replaced by the sum of smaller configuration files within configurations.dev. The intent is that we should be able to add small/targeted configuration files to test isolated parts of the uss_qualifier system which will allow us to iterate faster while still maintaining the same CI coverage we have today.
Since multiple configurations are being run, a separate report name is used for each one. This will incidentally allow easier identification of problems as the volume of content to search will be smaller (as long as it is clear in which test run the problem occurred).
Also, in this effort to make the configurations in configurations.dev more accessible, this PR adds a $content_schema key to indicate what schemas objects are intending to follow. This is not used anywhere programmatic and is intended to be a fancy comment to human readers.
A few random bugs occurring in off-nominal cases are fixed in common_dictionary_evaluator.py.