Repository navigation
Re-enable NSKIP003 - #861
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes Nanvix-specific NSKIP003 skip guards from several tests, with the intent to rely on test.support.requires_subprocess()/has_subprocess_support to skip subprocess-dependent tests on platforms where subprocess is unsupported (e.g., Nanvix).
Changes:
- Removed
@unittest.skipIf(support.is_nanvix, "NSKIP003: no subprocess support")guards from selected tests intest_os,test_shutil,test_exceptions, andtest_json. - Standardized skip behavior to prefer
requires_subprocess()where present (notablytest_json/test_tool.py).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Lib/test/test_shutil.py | Removes Nanvix skip around test_stty_match() (but this test still calls subprocess directly and needs a subprocess guard). |
| Lib/test/test_os.py | Removes Nanvix skip around TermsizeTests.test_stty_match() (but this test still calls subprocess directly and needs a subprocess guard). |
| Lib/test/test_json/test_tool.py | Drops redundant Nanvix skip since the class is already decorated with @support.requires_subprocess(). |
| Lib/test/test_exceptions.py | Drops Nanvix skips on tests that use script_helper.assert_python_*() (these already raise SkipTest when subprocess is unsupported). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # NSKIP003 https://github.com/nanvix/cpython/issues/471 | ||
| @unittest.skipIf(support.is_nanvix, | ||
| "NSKIP003: no subprocess support") | ||
| def test_stty_match(self): |
| @unittest.skipUnless(os.isatty(sys.__stdout__.fileno()), "not on tty") | ||
| @unittest.skipUnless(hasattr(os, 'get_terminal_size'), | ||
| 'need os.get_terminal_size()') | ||
| # NSKIP003 https://github.com/nanvix/cpython/issues/471 | ||
| @unittest.skipIf(support.is_nanvix, | ||
| "NSKIP003: no subprocess support") | ||
| def test_stty_match(self): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Lib/test/test_os.py:4020
test_stty_match()usessubprocess.check_output()but isn’t decorated with@support.requires_subprocess(). After removing the Nanvix skip, this test can run on platforms without subprocess/fork/exec support (Nanvix, WASI, Emscripten) and error instead of skipping.
def test_stty_match(self):
"""Check if stty returns the same results
Lib/test/test_shutil.py:3227
test_stty_match()callssubprocess.check_output()directly, but after removing the Nanvix guard it will now run on platforms wheresupport.has_subprocess_supportis false (e.g., Nanvix), and can fail with ENOSYS/NotImplementedError rather than being skipped. Add@support.requires_subprocess()so the test is consistently skipped when subprocess isn’t supported.
@unittest.skipUnless(os.isatty(sys.__stdout__.fileno()), "not on tty")
@unittest.skipUnless(hasattr(os, 'get_terminal_size'),
'need os.get_terminal_size()')
def test_stty_match(self):
Redundant: requires_subprocess() already skips these. subprocess itself is still unsupported.
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Lib/test/test_shutil.py:3227
test_stty_match()invokessubprocess.check_output()directly but no longer has a Nanvix/subprocess guard. Sincetest.support.has_subprocess_supportis false on Nanvix/wasm platforms, this should be decorated with@support.requires_subprocess()to avoid hard failures when subprocess is unavailable.
@unittest.skipUnless(os.isatty(sys.__stdout__.fileno()), "not on tty")
@unittest.skipUnless(hasattr(os, 'get_terminal_size'),
'need os.get_terminal_size()')
def test_stty_match(self):
Lib/test/test_os.py:3998
TermsizeTests.test_stty_match()usessubprocess.check_output()but the Nanvix-specific skip was removed and there is no@support.requires_subprocess()decorator. On platforms without subprocess support (Nanvix/wasm), this will raise before the existingFileNotFoundError/CalledProcessErrorskip logic can run. Add@support.requires_subprocess()to make the skip consistent withtest.support.has_subprocess_support.
def test_stty_match(self):
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Closes #471
What
Remove NSKIP003 guards in test_os / test_exceptions / test_shutil / test_json.
Why
Redundant: requires_subprocess() already skips these. subprocess itself is still unsupported.
Verified the full lifecycle on Linux.