Skip to content

A few minor fixes in roottest #20832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
hageboeck wants to merge 3 commits into
base: master
Choose a base branch
from
Open

A few minor fixes in roottest #20832

hageboeck wants to merge 3 commits into from
+4 −2

Conversation

@hageboeck
Copy link
Member

@hageboeck hageboeck commented Jan 9, 2026 •
edited
Loading

While tracing the problems addressed in #20818, I stumbled over a few small errors / warnings, which are fixed here.

What's a bit worrying is the test roottest-root-io-newstl-nolib:

When it was ported and re-enabled by @linev, it was already failing, see 1795466.
Afterwards, a non-existing function call was introduced in #18859, e60640c, such that the macro couldn't even be interpreted, any more. Due to the WILLFAIL annotation, this wasn't noticed.

Now we are back to the macro being interpreted, and failing during TTree::Scan with:

2336: ********************************************************
2336: *    Row   * Instance *       split99.fPairFlInt.first *
2336: ********************************************************
2336: *        0 *        0 *                              2 *
2336: *        1 *        0 *                              1 *
2336: *        1 *        1 *                              3 *
2336: *        2 *free(): invalid pointer

What should be the best action? Create an issue or disable/remove the test? @dpiparo @pcanal maybe?

@hageboeck hageboeck requested a review from guitargeek January 9, 2026 14:43
@hageboeck hageboeck self-assigned this Jan 9, 2026
@hageboeck hageboeck requested review from dpiparo and removed request for dpiparo January 9, 2026 14:44
@hageboeck hageboeck mentioned this pull request Jan 9, 2026
Draft
guitargeek
guitargeek approved these changes Jan 9, 2026
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

LGTM!

pcanal
pcanal approved these changes Jan 9, 2026
View reviewed changes
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

Thanks.

@github-actions
Copy link

github-actions bot commented Jan 9, 2026 •
edited
Loading

Test Results

    23 files      23 suites   5d 12h 4m 30s ⏱️
 3 814 tests  3 812 ✅   0 💤 2 ❌
80 399 runs  80 297 ✅ 100 💤 2 ❌

For more details on these failures, see this check.

Results for commit 90fa494.

♻️ This comment has been updated with latest results.

@hageboeck
[roottest] Unset list of defines before adding new macro tests.
de9cc11 
Macros are invoked with -e "#define xxx", but this list was not cleared,
leading to a growing list of repeated defines when a test consists of
multiple macros.
@hageboeck
[roottest] Fix an inconsistent override warning.
d676bb4
@hageboeck
[roottest] Replace a function that doesn't exist.
90fa494 
Since roottest-root-io-newstl-nolib is marked WILLFAIL, it was not
noticed in root-project#18859 that e60640c introduced a function that does not
exist.
The test still fails, but that's a different issue to be investigated.
@pcanal
Copy link
Member

pcanal commented Jan 12, 2026

What should be the best action? Create an issue or disable/remove the test?

The 'best' action is to solve the problem of course :). Short of this we should make sure that there is an issue tracking the underlying problem (creating if not present yet, it could also be in JIRA) and keep the test as WILL FAIL (the intent there is that is a cheap way to be notified when we 'unintentionally' fix the issue. (The downside is indeed, the test decay where it can not tell whether we are seeing the original issue or something else :( ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcanal pcanal pcanal approved these changes

@guitargeek guitargeek guitargeek approved these changes

@bellenot bellenot Awaiting requested review from bellenot bellenot is a code owner

@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner

Assignees

@hageboeck hageboeck

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hageboeck @pcanal @guitargeek