[action] [PR:25930] [conditional_mark] Fix nodeid normalization so skips apply when rootdir floats above tests/#1328
Merged
Conversation
…ir floats above tests/
Fixes #25929
### Description of PR
`conditional_mark` matches each test's `item.nodeid` against the tests-mark-conditions files, whose keys are relative to the `tests/` directory (e.g. `platform_tests/api`). Before matching, `pytest_collection_modifyitems` normalizes the nodeid by stripping `basename(rootpath) + "/"` (added for pytest 9.0+ in #22944).
That normalization breaks when pytest's `rootdir` is not the `tests/` directory. When the harness is invoked with a path argument outside `tests/` (e.g. `run_tests.sh` passes `--inventory ../ansible/veos_vtb`), pytest 9.0 floats `rootdir` up to the sonic-mgmt checkout root, so `item.nodeid` becomes `tests/platform_tests/api/...`. `root_prefix` is then `basename(<checkout>)/`, which does not prefix `tests/...`; the strip no-ops, the `tests/` survives, and every `nodeid.startswith(<key>)` fails. Result: **no conditional mark is applied at all** (`Found match: 0`, `skipped: 0`).
Practically, on a virtual-switch (`asic_type: vs`) DUT the `platform_tests/api: skip if asic_type in ['vs']` rule stops firing, so those tests run and error with `No module named 'sonic_platform'` instead of skipping with "Unsupported platform API".
This PR adds a second normalization step: after stripping `basename(rootpath)`, also strip a leading `tests/` so nodeids match the `tests/`-relative rule keys regardless of where pytest resolved `rootdir`.
#### Type of change
- [ ] Bug fix
- [ ] Testbed and Framework (new/improvement)
- [ ] New Test case
- [x] Test case enhancement
- [ ] Skipped for non-supported platforms
### Back port request
- [ ] 202305
- [ ] 202311
- [ ] 202405
- [ ] 202411
- [ ] 202505
### Approach
#### What is the motivation for this PR?
Conditional skips/xfails are silently ignored whenever pytest's `rootdir` resolves above `tests/` (which happens with the standard `run_tests.sh` invocation that passes `../`-relative paths). This causes false ERRORs for tests that are supposed to skip — most visibly the `platform_tests/api` suite on virtual-switch DUTs, but it affects every conditional rule in that run mode.
#### How did you do it?
In `pytest_collection_modifyitems`, after the existing `basename(rootpath)` strip, also strip a leading `tests/` from the nodeid before matching against the condition keys.
#### How did you verify/test it?
On a virtual-switch (`asic_type: vs`) testbed via `run_tests.sh` (which passes `--inventory ../ansible/veos_vtb`, i.e. the rootdir-floats-up case):
| Test | Before | After |
| --- | --- | --- |
| `platform_tests/api/test_watchdog.py` | 8 errors (`No module named 'sonic_platform'`) | **8 skipped** ("Unsupported platform API") |
| `platform_tests/api/test_psu.py` | errors | **14 skipped** ("Unsupported platform API") |
| `platform_tests/sfp/test_sfputil.py` | errors | `asic_type in ['vs']` **xfail** rule fires (4 xfailed) |
The `sfp` case exercises a different conditions file and an `xfail` (not `skip`) rule, confirming the fix restores conditional-mark matching globally rather than only for `platform_tests/api`.
`flake8 --max-line-length=120` clean.
### Documentation
- [ ] Yes
- [x] No
Signed-off-by: Sonic Build Admin <sonicbld@microsoft.com>
Collaborator
Author
|
Original PR: sonic-net/sonic-mgmt#25930 |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
21 tasks
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #25929
Description of PR
conditional_markmatches each test'sitem.nodeidagainst the tests-mark-conditions files, whose keys are relative to thetests/directory (e.g.platform_tests/api). Before matching,pytest_collection_modifyitemsnormalizes the nodeid by strippingbasename(rootpath) + "/"(added for pytest 9.0+ in #22944).That normalization breaks when pytest's
rootdiris not thetests/directory. When the harness is invoked with a path argument outsidetests/(e.g.run_tests.shpasses--inventory ../ansible/veos_vtb), pytest 9.0 floatsrootdirup to the sonic-mgmt checkout root, soitem.nodeidbecomestests/platform_tests/api/....root_prefixis thenbasename(<checkout>)/, which does not prefixtests/...; the strip no-ops, thetests/survives, and everynodeid.startswith(<key>)fails. Result: no conditional mark is applied at all (Found match: 0,skipped: 0).Practically, on a virtual-switch (
asic_type: vs) DUT theplatform_tests/api: skip if asic_type in ['vs']rule stops firing, so those tests run and error withNo module named 'sonic_platform'instead of skipping with "Unsupported platform API".This PR adds a second normalization step: after stripping
basename(rootpath), also strip a leadingtests/so nodeids match thetests/-relative rule keys regardless of where pytest resolvedrootdir.Type of change
Back port request
Approach
What is the motivation for this PR?
Conditional skips/xfails are silently ignored whenever pytest's
rootdirresolves abovetests/(which happens with the standardrun_tests.shinvocation that passes../-relative paths). This causes false ERRORs for tests that are supposed to skip — most visibly theplatform_tests/apisuite on virtual-switch DUTs, but it affects every conditional rule in that run mode.How did you do it?
In
pytest_collection_modifyitems, after the existingbasename(rootpath)strip, also strip a leadingtests/from the nodeid before matching against the condition keys.How did you verify/test it?
On a virtual-switch (
asic_type: vs) testbed viarun_tests.sh(which passes--inventory ../ansible/veos_vtb, i.e. the rootdir-floats-up case):platform_tests/api/test_watchdog.pyNo module named 'sonic_platform')platform_tests/api/test_psu.pyplatform_tests/sfp/test_sfputil.pyasic_type in ['vs']xfail rule fires (4 xfailed)The
sfpcase exercises a different conditions file and anxfail(notskip) rule, confirming the fix restores conditional-mark matching globally rather than only forplatform_tests/api.flake8 --max-line-length=120clean.Documentation
Signed-off-by: Sonic Build Admin sonicbld@microsoft.com