Skip to content

test: tighten the existing tests and make their fixtures robust - #1823

Merged
ianna merged 4 commits into
scikit-hep:mainfrom
ariostas:test/existing-tests
Nov 7, 2026
Merged

ianna merged 4 commits into
scikit-hep:mainfrom
ariostas:test/existing-tests

Conversation

@ariostas

@ariostas ariostas commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

🤖 AI text below 🤖

Part of the split of #1814 (the list is in #1834). These are the changes to existing tests from #1814 that don't depend on any of its fixes: they pass on main. Changes that do depend on a fix are in that fix's PR.

  • Real checks: tests assert values instead of only running the code, and checks that had been dropped are restored. For example, test_0043 reads all 14 files again and test_0603 checks delayed opening of local files.
  • Skips: tests that were skipped but work now run again (test_0927, test_0965, and others).
  • Cleanup: open files are closed (with with uproot.open), and repeated tests are parametrized.
  • Fixtures (tests/conftest.py)
    • Each XRootD test server gets its own admin and pid path, so concurrent servers under pytest-xdist don't collide on /tmp/.xrootd.
    • The servers wait until they accept connections, instead of sleeping for 2 seconds.
    • reset_classes puts the original model classes back afterward, the same change as in fix: correct errors found while testing the interpretations #1824.
  • Deleted: test_1146, which was always skipped because its server requires authentication.

The five coverage PRs (listed in #1814) are based on this one. Its branch is also pushed to this repository as test/existing-tests, so that those PRs show only their own tests.

@codecov

codecov Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.04%. Comparing base (24f1c3f) to head (6de8cb1).

Additional details and impacted files

see 14 files with indirect coverage changes

@ariostas
ariostas marked this pull request as ready for review November 6, 2026 14:47
Changes to existing tests from scikit-hep#1814 that don't depend on its fixes: they
check values instead of only running, restore checks that had been dropped,
close the files they open, and run concurrently (each XRootD server has its
own admin path and waits until it accepts connections, and reset_classes puts
the original model classes back).

Assisted-by: Claude Code:claude-opus-5-5
@ariostas
ariostas force-pushed the test/existing-tests branch from c192219 to 5253a21 Compare November 6, 2026 15:09
Six tests (of Future.result timeouts, ThreadPoolExecutor.shutdown, the order
of LRUCache, and Library equality) were deleted here in favor of tests in
scikit-hep#1829, scikit-hep#1830 and scikit-hep#1832, which are based on this PR; until those are merged
nothing else would cover them. They are now deleted together with the tests
that replace them. A comment no longer points to a file of scikit-hep#1828.

Assisted-by: Claude Code:claude-opus-5-5
MET_pt is in one TBasket, so with steps of 15 entries the later steps of a
file had nothing to request, and the XRootD source then sent an empty vector
read, which failed now and then. scikit-hep#1825 stops sending it; until then, these
tests iterate in one step per file.

Assisted-by: Claude Code:claude-opus-5-5

@ianna ianna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ariostas -- Thanks! LGTM

@ianna
ianna enabled auto-merge (squash) November 7, 2026 08:50
@ianna
ianna merged commit 39ff721 into scikit-hep:main Nov 7, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/test PR title type: test (set automatically)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants