-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[CI] Cross-platform — Part 1: pytest markers and shared scaffolding (base) #5695
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
base: develop
Are you sure you want to change the base?
Changes from all commits
9242498
65ca558
1e32138
f08fe8b
ec875a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Skip changelog: CI/test-infrastructure foundation (no user-facing API change). Registers the windows_ci and arm_ci pytest markers in pyproject.toml, teaches AppLauncher to recognize them in argv so they do not leak into Isaac Sim's argparse, and moves the AssetConverterBase USD scratch dir from hardcoded /tmp/IsaacLab to tempfile.gettempdir() for cross-platform compatibility. Test tagging and the cross-platform workflow files (arm-ci.yaml, windows-ci.yaml) ship in follow-up PRs. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1193,12 +1193,18 @@ def _create_app(self): | |||||||||||||||||||||||||||||
| sys.stdout = open(os.devnull, "w") # noqa: SIM115 | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # pytest may have left some things in sys.argv, this will check for some of those | ||||||||||||||||||||||||||||||
| # do a mark and sweep to remove any -m pytest and -m isaacsim_ci and -c **/pyproject.toml | ||||||||||||||||||||||||||||||
| # do a mark and sweep to remove any -m pytest, -m isaacsim_ci, -m windows_ci, -m arm_ci, | ||||||||||||||||||||||||||||||
| # and -c **/pyproject.toml | ||||||||||||||||||||||||||||||
| indexes_to_remove = [] | ||||||||||||||||||||||||||||||
| for idx, arg in enumerate(sys.argv[:-1]): | ||||||||||||||||||||||||||||||
| if arg == "-m": | ||||||||||||||||||||||||||||||
| value_for_dash_m = sys.argv[idx + 1] | ||||||||||||||||||||||||||||||
| if "pytest" in value_for_dash_m or "isaacsim_ci" in value_for_dash_m: | ||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||
| "pytest" in value_for_dash_m | ||||||||||||||||||||||||||||||
| or "isaacsim_ci" in value_for_dash_m | ||||||||||||||||||||||||||||||
| or "windows_ci" in value_for_dash_m | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are these new annotators necessary for this check? I think this was initially added for the isaac sim pipeline to work properly by making sure the correct arguments gets passed over. will that also be impacted by the windows and arm annotations?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's for pytest selecting tests, which uses the same mechanism as isaacsim_ci tag. While this is not quite scalable at the moment, some investigation into argparse refactor should help. e.g. to only pass appLauncher recognized arguments to it.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we not running the the whole batch of tests on windows? Would it make sense to make a specific test shard just for windows? (that would remove the annotation) |
||||||||||||||||||||||||||||||
| or "arm_ci" in value_for_dash_m | ||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||
|
Comment on lines
+1202
to
+1207
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||
| indexes_to_remove.append(idx) | ||||||||||||||||||||||||||||||
| indexes_to_remove.append(idx + 1) | ||||||||||||||||||||||||||||||
| if arg.startswith("--config-file=") and "pyproject.toml" in arg: | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
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.
Is this better than a specific shard?
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.
Not exactly sure what a shard means here. Does it mean selecting the tests at workflow level? If so, I am trying to make it obvious at test level all the configuration.
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.
Here we'd mark specific tests in our CI for windows / arm right? What I'm wondering is if we setup a specific job for windows that would list the test files we want to run. It seems weird to me to only mark a couple of tests here and here across the code base for these platform tests. Or it's not how this works and these markers are used to mark files instead?
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.
Yes both. It can opt in at file level or at per test level. mainly two problems