-
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] Switch default ASTM NETRID to v22a and reorganize CI tests #119
Conversation
b1112ab
to
aebf8b6
Compare
d551b6d
to
77ff647
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.
Really nice to have all that running properly together :)
IMO all comments are nits, except for the separate PR disabling the atproxy.
In addition to review comments, some nits:
- in
monitoring/mock_uss
, maybe renamerun_locally_riddp.sh
torun_locally_riddp_v19.sh
for clarity? (same respectively forrun_locally_ridsp.sh
) - In the CI/local_test, rather than having the uspace test ran, having the individual suites ran separately could be nice (would help identify in GH Actions directly the failures). But that's not really the subject of this PR.
@@ -0,0 +1,43 @@ | |||
name: Local tests |
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.
Does this definition really belong here? My understanding is that in suites/
we have "low-level" / base test definitions, that we can instrument from definitions in configurations/
. So I have the feeling this would belong in configurations/dev
, it does not feel like a dev
categorty belong here. (I'm not 100% confident I'm right on this though)
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.
The dev suite was already existing for local_test to create dedicated suites which include test data generation. Based on your comment and based on a discussion with @BenjaminPelletier few weeks ago, I took the opportunity to switch the repo defaults to RID v22a and updated the local_test to run the uspace suite to cover as many as possible tests. v19 stays as an isolated test. Please let me know what you think.
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.
Made an additional pass, LGTM!
This PR switches the default version of ASTM NETRID to v22a and updates the local_test to run the uspace test suite. In addition, the CI job will be split in two steps to cover U-Space Test Suite as well as the NETRID v19 separately. v19 has been preserved in order to exercise supporting multiple versions of the standard.
The github actions job and uss_qualifier manifests have been updated to run v19 tests and, U-Space related tests with properly configured mock_uss (v22a).
Please note that the issue reported in #28 seems to be due to the atproxy. I would like to suggest to disable it for few days in the CI to see if the 999 timeout still occurs. If not, that would demonstrate that we isolated the problem. This change is proposed in this PR and the atproxy client is replaced by an actual mock_uss when applicable. Please let me know if you would like to approach this differently.-> move to an upcoming PR.