Skip to content

fix: stop a <testsuites> wrapper lending a nested suite's run markers - #10

Open
JingHao-Leon wants to merge 2 commits into
maxgregaabig:mainfrom
JingHao-Leon:fix/junit-wrapper-marker-inheritance
Open

JingHao-Leon wants to merge 2 commits into
maxgregaabig:mainfrom
JingHao-Leon:fix/junit-wrapper-marker-inheritance

Conversation

@JingHao-Leon

@JingHao-Leon JingHao-Leon commented Oct 19, 2026 •

Copy link
Copy Markdown

What this changes

A <testsuites> wrapper no longer lends the commit and attempt markers of a suite nested inside it to a suite that carries no markers of its own. Such a suite now falls back to the documented defaults and records the missing metadata warning, which is what the README already describes.

Why

src/flakeledger/junit.py:74 — _read_run_metadata reads commit and attempt with elem.get(...), which looks at the element itself, but reads the <property> markers with elem.iter("property"), which descends into nested elements. The two lookups had different scopes: the attribute lookup stopped at the element, the property lookup did not.

src/flakeledger/junit.py:150 — when a suite has no markers of its own, parse_file fell back to _read_run_metadata(root). The root is the <testsuites> wrapper, so the recursive property lookup found the <property> elements of the suites nested inside it and reported them as the root's own markers. The unmarked suite was given that commit and attempt, and because the fallback reported no missing metadata, the warning block at src/flakeledger/junit.py:155-160 was skipped.

The result was silently wrong: a suite with no run identity was attributed to a sibling suite's commit, the missing metadata warning was suppressed, and a test that never flaked was reported as a flake and charged for a flaky event that never happened.

The attribute marker style (<testsuite commit="..." attempt="...">) was unaffected, because elem.get does not descend. Only the <property> style, which the README documents as the second accepted style, hit this.

Fixes #9

Before

Fixture, the beta suite carries no run markers:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="alpha">
    <properties>
      <property name="commit" value="aaaaaa"/>
      <property name="attempt" value="1"/>
    </properties>
    <testcase classname="tests.demo" name="test_p" time="1.0"/>
  </testsuite>
  <testsuite name="beta">
    <testcase classname="tests.demo" name="test_q" time="1.0">
      <failure message="boom"/>
    </testcase>
  </testsuite>
</testsuites>
$ PYTHONPATH=src python -m flakeledger ingest demo/
cases 3
commits 1
commit aaaaaa
case aaaaaa attempt=1 tests.demo.test_p passed time=1.000s
case aaaaaa attempt=1 tests.demo.test_q failed time=1.000s
case aaaaaa attempt=2 tests.demo.test_q passed time=1.000s

$ PYTHONPATH=src python -m flakeledger classify demo/
policy single_fail=undetermined
count flake 1
count stable 1
class tests.demo.test_p @ aaaaaa stable attempts=1 pass=1 fail=0 skip=0 (all 1 attempt(s) passed or skipped, no failures)
class tests.demo.test_q @ aaaaaa flake attempts=2 pass=1 fail=1 skip=0 (same commit produced 1 pass(es) and 1 failure(s) across 2 attempts)

$ PYTHONPATH=src python -m flakeledger cost demo/
ranked flaky test cost (highest first):
  1. tests.demo.test_q
     events=1 wasted_compute=0.017min dev_wait=15.0min
     compute_cost=0.0001 USD dev_cost=22.5000 USD total=22.5001 USD

total flaky cost 22.5001 USD

No warning is printed, even though the beta suite has neither marker.

After

$ PYTHONPATH=src python -m flakeledger ingest demo/
cases 3
commits 2
commit aaaaaa
commit unknown-commit
case aaaaaa attempt=1 tests.demo.test_p passed time=1.000s
case aaaaaa attempt=2 tests.demo.test_q passed time=1.000s
case unknown-commit attempt=1 tests.demo.test_q failed time=1.000s
warning run-aaaaaa-attempt1.xml: suite 'beta' missing commit or attempt metadata, used defaults (commit=unknown-commit, attempt=1)

$ PYTHONPATH=src python -m flakeledger classify demo/
policy single_fail=undetermined
count stable 2
count undetermined 1
class tests.demo.test_p @ aaaaaa stable attempts=1 pass=1 fail=0 skip=0 (all 1 attempt(s) passed or skipped, no failures)
class tests.demo.test_q @ aaaaaa stable attempts=1 pass=1 fail=0 skip=0 (all 1 attempt(s) passed or skipped, no failures)
class tests.demo.test_q @ unknown-commit undetermined attempts=1 pass=0 fail=1 skip=0 (single failing attempt, no rerun to compare, undetermined)

$ PYTHONPATH=src python -m flakeledger cost demo/
ranked flaky test cost (highest first):
  none: no test was classified as a flake

The beta suite falls back to unknown-commit, its failing attempt is the only observation for that commit, so it is undetermined and not charged.

Verification

PYTHONPATH=src python -m pytest -q, with the source at 257573e and the new tests applied:

FAILED tests/test_junit.py::TestJunitParsing::test_suite_without_markers_does_not_inherit_a_sibling_suite_markers
1 failed, 23 passed

With the fix:

24 passed

PYTHONPATH=src python -m unittest discover -s tests reports FAILED (failures=1) before the fix and Ran 24 tests / OK after it.

python -m compileall -q src passes and PYTHONPATH=src python -m flakeledger --help runs.

The output over the bundled samples is unchanged: ingest still reports 25 cases over 4 commits, classify still reports 3 flakes, 2 genuine failures and 9 stable, and cost still reports a total flaky cost of 67.5008 USD.

Checklist

  • python -m compileall -q src passes
  • python -m pytest -q passes
  • Docs updated (README, CHANGELOG) where behavior changed

CHANGELOG.md gains a ### Fixed entry under [Unreleased]. The README needed no change: it already documents the fallback to commit=unknown-commit, attempt=1 with a warning, and this makes the code do that.

CI status

The CI workflow for this head has not run. GitHub recorded the run with
conclusion: action_required — the run is blocked awaiting approval (for
workflows triggered from a fork, GitHub requires a maintainer to approve them
before they execute) — and gh pr checks reports no checks for this branch.

So there is no upstream CI result to report here. The test counts above are
from a local run against the patched tree.

This change was prepared with the assistance of an AI coding agent.

A <testsuites> wrapper holding two suites, one with <property> run
markers and one without, shows the unmarked suite borrowing the marked
sibling's commit. The unmarked suite should fall back to the documented
defaults and warn instead.

Co-Authored-By: JingHao-Leon <102573344+JingHao-Leon@users.noreply.github.com>
_read_run_metadata looked up commit/attempt attributes on the element
itself, but looked up <property> markers with ElementTree.iter, which
descends into nested elements. A <testsuites> wrapper therefore reported
the first nested suite's markers as its own, and parse_file's root level
fallback handed them to any suite that carried no markers of its own.

The result was silently wrong: a suite with no run identity was
attributed to a sibling's commit, the missing metadata warning was
suppressed, and a test that never flaked could be reported as a flake
and charged for it.

Restrict the root level fallback to the markers the root carries itself,
so a suite without markers falls back to the documented defaults and
warns, as documented.

Co-Authored-By: JingHao-Leon <102573344+JingHao-Leon@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parser attributes an unmarked suite to a sibling suite's commit inside a <testsuites> wrapper

1 participant