Isolate release-tooling tests from each other's progress files by bjlittle · Pull Request #7278 · SciTools/iris · GitHub
Skip to content

Isolate release-tooling tests from each other's progress files - #7278

Open
bjlittle wants to merge 2 commits into
SciTools:mainfrom
bjlittle:fix-release-test-cwd-race
Open

bjlittle wants to merge 2 commits into
SciTools:mainfrom
bjlittle:fix-release-test-cwd-race

Conversation

@bjlittle

Copy link
Copy Markdown
Member

🤖 Agentic pull request

This was written by Claude (Opus 5), driven by @bjlittle. The commit carries a
Co-Authored-By trailer. Please review it as you would any other contribution.

Found while investigating the tests (py3.14) failure on #7276, which was
unrelated to that pull request.

The failure

tools/test_release_do_nothing.py::TestMergeBack::test_next_patch_file failed
on one job and no other. Re-running the identical commit passed. That pattern is
the diagnosis: it is a race between tests, not a bug in the code under test.

What races

IrisRelease.merge_back writes a progress file for the next patch. Unlike
every other nothing progress file, it is named after that patch instead of
being date-stamped — tools/release_do_nothing.py:1292:

next_patch_stem = self._get_file_stem().with_stem(next_patch_str)

nothing.Progress._get_file_stem resolves .nothing/ against Path().cwd(),
which every pytest-xdist worker shares. So the directory is the only thing
distinguishing one test's progress file from another's, and it does not vary.

Two tests reach that line with the same version, and so with the same path,
.nothing/v1_1_1.json:

  • TestMergeBack::test_branches, in its more_patches parametrisation
  • TestMergeBack::test_next_patch_file

Both set git_tag = "v1.0.1" and patch_min_max_tag = ("v1.0.1", "v1.2.1"),
which makes the next patch v1.1.1 in each case.

It is a race rather than a plain clash because the writes are not atomic and the
reads are not guarded:

  • NextPatch(...) writes the JSON with write_text and then verifies it by
    loading it straight back.
  • Its logger opens the sibling .log with mode="w", truncating it.
  • test_next_patch_file loads the same path and asserts on its contents.

pyproject.toml sets no --dist, so xdist uses dynamic scheduling. Whether
those two tests land on the same worker varies from run to run, which is exactly
the intermittency observed.

The fix

A module-level autouse fixture giving each test its own working directory.

Autouse is the right scope rather than patching the two colliding tests:
_get_file_stem runs during __post_init__, so every construction of
IrisRelease is exposed to the shared directory, not just today's two
offenders.

Moving the working directory is safe here. All three subprocess calls in
release_do_nothing.py are the git helpers (_git_remote_v,
_git_remote_get_url, _git_ls_remote_tags), each already mocked by an autouse
fixture; every other path the module builds is anchored to Path(__file__).

It also stops the suite writing .nothing/ into the working copy as a side
effect — which is why **/.nothing is in .gitignore (.gitignore:85). On
main, pytest tools leaves .nothing/v1_1_1.json and v1_1_1.log in the
repository root. With this change it leaves nothing behind.

On testing a race

The failure is scheduling-dependent and cannot be reproduced on demand, so
test_progress_files_are_isolated asserts the isolation that prevents it rather
than the failure itself.

It is a real regression test: with the fixture body removed it fails, and names
the repository root in the message.

E  AssertionError: assert False
E   +  where False = is_relative_to(PosixPath('/tmp/pytest-of-.../test_progress_files_are_isolat0'))
E   +    where is_relative_to = PosixPath('/.../iris/.nothing/IrisRelease_20260911-105227').is_relative_to

Testing

pytest tools gives 122 passed, 1 skipped, against 121 passed, 1 skipped on
main — the one addition is the new test. Passes both serially and under
-n auto, and leaves no .nothing/ in the repository.

Note that pytest tools needs the nothing package, which is installed only by
noxfile.py and appears in no requirements or lock file. That is #7277, raised
separately for the core developers to rule on; it is an observation, not a
blocker for this.

Why this targets main

This is a repository-wide CI fix, not part of the merge/concatenate programme
that the greenfield feature branch carries.

greenfield needs it too — it inherits the same test module and the same
failure. It should pick it up by mergeback from main, not a cherry-pick,
so the branch keeps a single ancestry and does not carry a duplicate commit that
would have to be resolved later. greenfield descends from main, so the merge
is clean. @bjlittle and I will raise that mergeback once this lands.

bjlittle and others added 2 commits September 11, 2026 10:53
`IrisRelease.merge_back` writes a progress file for the *next* patch,
named after that patch rather than date-stamped like every other
`nothing` progress file:

    next_patch_stem = self._get_file_stem().with_stem(next_patch_str)

`nothing.Progress._get_file_stem` resolves `.nothing/` against
`Path().cwd()`, which every `pytest-xdist` worker shares.  Two tests
reach that code with the same version -- `TestMergeBack::test_branches`
in its `more_patches` parametrisation, and `test_next_patch_file` --
so both resolve to the one `.nothing/v1_1_1.json`.

That is a race, not a conflict.  `NextPatch(...)` writes the JSON
non-atomically and then verifies by reloading it, and its logger opens
the sibling `.log` with `mode="w"`.  `test_next_patch_file` loads the
same path back and asserts on its contents.  With no `--dist` setting
in `pyproject.toml`, xdist schedules dynamically, so whether the two
land on the same worker varies run to run: the observed `py3.14` failure
on SciTools#7276 reproduced on no other job, and the identical commit passed on
re-run.

Give each test its own working directory.  An autouse fixture is the
right scope because `_get_file_stem` runs during `__post_init__`, so
every construction of `IrisRelease` is exposed, not just the two tests
that collide today.  All three `subprocess` calls in
`release_do_nothing.py` are the git helpers already mocked by autouse
fixtures, and every other path it builds is anchored to `Path(__file__)`,
so moving the working directory reaches nothing else.

This also stops the suite writing `.nothing/` into the repository as a
side effect -- the reason `**/.nothing` sits in `.gitignore`.

The race cannot be reproduced on demand, so the accompanying test
asserts the isolation that prevents it instead of the failure itself.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

@bjlittle

Copy link
Copy Markdown
Member Author

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant