Skip to content

[BUG] mute stderr in QuickTester.run_tests, and fix np.all generator checks - #586

Merged
fkiraly merged 2 commits into
sktime:mainfrom
yash-sangwan:fix/quicktester-stderr-mute-and-np-all
Aug 25, 2026
Merged

[BUG] mute stderr in QuickTester.run_tests, and fix np.all generator checks#586
fkiraly merged 2 commits into
sktime:mainfrom
yash-sangwan:fix/quicktester-stderr-mute-and-np-all

Conversation

@yash-sangwan

Copy link
Copy Markdown
Member

Reference Issues/PRs

Spun out of the sktime test framework refactor (#10647), specifically PR #10862 which inherits BaseFixtureGenerator and QuickTester from skbase.testing. @fkiraly suggested upstreaming the StderrMute change there.

What does this implement/fix? Explain your changes.

This introduces two independent fixes in skbase/testing/test_all_objects.py, both found while making sktime inherit this test framework.

1. run_tests only muted stdout

Currently, run_tests wraps each test in StdoutMute(active=verbose < 2), so anything a test writes to stderr still reaches the log at verbose=0. sktime has been muting both streams since PR #8799, so inheriting run_tests unchanged would have silently dropped the stderr half. StderrMute already shipped in skbase.utils.stderr_mute, it was just never used here.

Measured before and after:

stdout leak stderr leak
before, verbose=0 0 1
after, verbose=0 0 0
after, verbose=2 1 1

verbose=2 still shows both streams, so the escape hatch is unchanged.

2. Three checks passed a generator expression to np.all

For example:

if not np.all(isinstance(x, str) for x in obj):

Without brackets, this is a generator expression. np.all receives a single generator object rather than an array of booleans. NumPy treats it as one scalar object and applies truthiness, and every generator object is truthy. The result is unconditionally True, the generator is never consumed, and the check never fires.

Affected areas:

  • QuickTester._check_none_str_or_list_of_str: tests_to_run, fixtures_to_run, and related args were effectively unvalidated.
  • Both checks in TestAllObjects.test_create_test_instances_and_names.

There is a secondary bug in the last of these. It read:

assert np.all(isinstance(name, names) for name in names), "... all must be strings"

Since names is a list of strings, not a type, isinstance(name, names) is invalid and raises a TypeError. The generator bug was masking it. Adding brackets alone would have converted a silently passing assert into a hard TypeError on every object under test, so this is corrected to isinstance(name, str) to match the assertion message.

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

  • Whether muting stderr by default at verbose < 2 is the desired behavior, or whether it should be opt-in.
  • The names to str correction in test_create_test_instances_and_names. It changes what that assert actually checks since it has never truly executed before.

…checks

Two fixes, both found while refactoring the sktime test framework to
inherit from skbase.

1. `run_tests` muted stdout only, so anything a test wrote to stderr still
   reached the log at `verbose < 2`. It now mutes both streams, matching what
   `sktime` does downstream. `StderrMute` already shipped in
   `skbase.utils.stderr_mute` but was never used here.

2. Three checks passed a generator expression to `np.all` instead of a list.
   `np.all` receives the generator object rather than an array of booleans, and
   every generator object is truthy, so the checks always passed and their
   bodies never ran.

   - `_check_none_str_or_list_of_str` never rejected invalid input, so
     `tests_to_run` and friends were unvalidated.
   - the two checks in `test_create_test_instances_and_names` never ran.

   The second of those also passed `names`, a list of str, as the second
   argument to `isinstance`, which is not a type. Wrapping the comprehension
   alone would raise TypeError, so it is corrected to `str`, which is what the
   assert message already says it checks.

Verified: full test suite is 1609 passed, 23 skipped, identical to main. An
object returning non str names or non instances is now caught by
`test_create_test_instances_and_names`, and was not before. At `verbose=0`
stderr no longer leaks, and `verbose=2` still shows both streams.
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.35%. Comparing base (306958d) to head (eca47f7).
⚠️ Report is 229 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
- Coverage   85.07%   84.35%   -0.72%     
==========================================
  Files          45       53       +8     
  Lines        3015     3995     +980     
==========================================
+ Hits         2565     3370     +805     
- Misses        450      625     +175     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and also for fixing the bugs in np.all.

Could you kindly add tests to prevent regressions? There should be fixtures that you can adapt. If it is too complex (e.g., resulting in 100s lines of code) we can omit it though.

@yash-sangwan

Copy link
Copy Markdown
Member Author

Just added! I managed to keep it very concise (under 50 lines) by using pytest's native capsys fixture to capture the stream outputs, so no complex mocking was needed.
I added regression tests for the StderrMute behavior and the QuickTester._check_none_str_or_list_of_str input validation. I omitted the test_create_test_instances_and_names dummy objects to respect your note about keeping the footprint small.
Lmk if this looks good to go.

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect.

@fkiraly
fkiraly merged commit eb2e089 into sktime:main Aug 25, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Adding new functionality implementing framework Implementing core skbase framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants