Skip to content

test: cover the interpretations - #1829

Merged
ianna merged 2 commits into
scikit-hep:test/existing-testsfrom
ariostas:test/interpretation-coverage
Nov 12, 2026
Merged

ianna merged 2 commits into
scikit-hep:test/existing-testsfrom
ariostas:test/interpretation-coverage

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 new tests of #1814 for the interpretations (containers, AwkwardForth, identification, the Python language, libraries, objects and arrays), apart from the regression tests of its fixes, which are in the fix PRs. All of them pass on main.

The base of this PR is test/existing-tests (#1823). It adds test files and deletes test_library_equality_is_safe of test_0016, which test_names_and_equality replaces. Once #1823 is merged, it will be rebased on main.

@github-actions github-actions Bot added the type/test PR title type: test (set automatically) label Nov 6, 2026
@ariostas
ariostas marked this pull request as ready for review November 6, 2026 14:47
@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 80.53%. Comparing base (e12d2a4) to head (8f105e8).

Additional details and impacted files

see 14 files with indirect coverage changes

@ariostas
ariostas force-pushed the test/interpretation-coverage branch from ef53184 to df3a29d Compare November 6, 2026 15:09
@ariostas
ariostas force-pushed the test/existing-tests branch from c192219 to 5253a21 Compare November 6, 2026 15:10
New tests from scikit-hep#1814, apart from the regression tests of its fixes.

Assisted-by: Claude Code:claude-opus-5-5
The check that a sequence isn't equal to a string passed for anything but
True. The test of Node.__str__ expected its wrong indentation, which scikit-hep#1824
fixes; it is now there, with the right one. The test of Library equality in
test_0016 is deleted here, as test_names_and_equality covers it.

Assisted-by: Claude Code:claude-opus-5-5
ariostas added a commit to ariostas/uproot5 that referenced this pull request Nov 6, 2026
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
@ariostas
ariostas force-pushed the test/interpretation-coverage branch from df3a29d to 8f105e8 Compare November 6, 2026 16:25
ianna pushed a commit that referenced this pull request Nov 7, 2026
* test: tighten the existing tests and make their fixtures robust

Changes to existing tests from #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

* test: keep the tests that the new coverage tests replace

Six tests (of Future.result timeouts, ThreadPoolExecutor.shutdown, the order
of LRUCache, and Library equality) were deleted here in favor of tests in
#1829, #1830 and #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 #1828.

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

* test: iterate over XRootD in one step per file

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. #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 for splitting the PR. It's easier to digest. Looks great!

@ianna
ianna merged commit 4c59767 into scikit-hep:test/existing-tests Nov 12, 2026
36 of 39 checks passed
@ariostas

Copy link
Copy Markdown
Member Author

🤖 AI text below 🤖

This was merged into test/existing-tests instead of main by mistake, so its changes aren't in main. #1841 has the same changes, rebased on main.

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