8000 Isolate test runs to avoid cross talks of Nextflow assets by muffato · Pull Request #4175 · nf-core/tools · GitHub
[go: up one dir, main page]
Skip to content

Isolate test runs to avoid cross talks of Nextflow assets - #4175

Merged
muffato merged 15 commits into
devfrom
test_schema_race_condition
Apr 9, 2026
Merged

Isolate test runs to avoid cross talks of Nextflow assets#4175
muffato merged 15 commits into
devfrom
test_schema_race_condition

Conversation

@muffato
@muffato muffato commented Apr 4, 2026 •
edited
Loading
Copy link
Copy Markdown
Member

I found a problem while working on my other PRs. tests/pipelines/test_schema.py:test_get_schema_path_name was intermittently failing and the funny thing is that the log was showing something about exoseq which is the other test (test_get_schema_path_name_notexist).

The error was only happenning on GitHub, which has the pytest-xdist plugin installed to run tests in parallel. I didn't get the error when running the tests (sequentially).
On GitHub, the error went away after a / few reruns.

It turns out there was a race condition between the tests as they both relied on Nextflow's assets directory. My fix is to:

  1. make nf_core/pipelines/list.py directly look into the expected location of the assets / pipeline.
  2. make tests use temporary assets directories, simply by extending the setUp method.

Original error below from https://github.com/nf-core/tools/actions/runs/23983530419/job/69951649385?pr=4147 :

============================= test session starts ==============================
platform linux -- Python 3.10.20, pytest-8.4.2, pluggy-1.6.0
rootdir: /home/runner/_work/tools/tools
configfile: pyproject.toml
plugins: asyncio-1.3.0, datafiles-3.0.1, xdist-3.8.0, workflow-2.1.0, textual-snapshot-1.1.0, syrupy-4.8.0, cov-7.0.0
asyncio: mode=auto, debug=False, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
created: 4/4 workers
4 workers [53 items]

.......F.............................................                    [100%]
=================================== FAILURES ===================================
_____________________ TestSchema.test_get_schema_path_name _____________________
[gw1] linux -- Python 3.10.20 /home/runner/_work/tools/tools/.venv/bin/python3

self = <tests.pipelines.test_schema.TestSchema testMethod=test_get_schema_path_name>

    def test_get_schema_path_name(self):
        """Get schema file from the name of a remote pipeline"""
>       self.schema_obj.get_schema_path("atacseq")

tests/pipelines/test_schema.py:89: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
nf_core/pipelines/schema.py:137: in get_schema_path
    self.pipeline_dir = nf_core.pipelines.list.get_local_wf(path, revision=revision)
nf_core/pipelines/list.py:70: in get_local_wf
    wfs.get_local_nf_workflows()
nf_core/pipelines/list.py:161: in get_local_nf_workflows
    wf.get_local_nf_workflow_details()
nf_core/pipelines/list.py:379: in get_local_nf_workflow_details
    self.commit_sha = str(repo.head.commit.hexsha)
.venv/lib/python3.10/site-packages/git/refs/symbolic.py:394: in commit
    return self._get_commit()
.venv/lib/python3.10/site-packages/git/refs/symbolic.py:298: in _get_commit
    obj = self._get_object()
.venv/lib/python3.10/site-packages/git/refs/symbolic.py:289: in _get_object
    return Object.new_from_sha(self.repo, hex_to_bin(self.dereference_recursive(self.repo, self.path)))
.venv/lib/python3.10/site-packages/git/refs/symbolic.py:169: in dereference_recursive
    hexsha, ref_path = cls._get_ref_info(repo, ref_path)
.venv/lib/python3.10/site-packages/git/refs/symbolic.py:279: in _get_ref_info
    return cls._get_ref_info_helper(repo, ref_path)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'git.refs.head.HEAD'>
repo = <git.repo.base.Repo '/home/runner/.nextflow/assets/nf-core/exoseq/.git'>
ref_path = 'refs/heads/master'

    @classmethod
    def _get_ref_info_helper(
        cls, repo: "Repo", ref_path: Union[PathLike, None]
    ) -> Union[Tuple[str, None], Tuple[None, str]]:
        """
        :return:
            *(str(sha), str(target_ref_path))*, where:
    
            * *sha* is of the file at rela_path points to if available, or ``None``.
            * *target_ref_path* is the reference we point to, or ``None``.
        """
        if ref_path:
            cls._check_ref_name_valid(ref_path)
    
        tokens: Union[None, List[str], Tuple[str, str]] = None
        repodir = _git_dir(repo, ref_path)
        try:
            with open(os.path.join(repodir, ref_path), "rt", encoding="UTF-8") as fp:  # type: ignore[arg-type]
                value = fp.read().rstrip()
            # Don't only split on spaces, but on whitespace, which allows to parse lines like:
            # 60b64ef992065e2600bfef6187a97f92398a9144                branch 'master' of git-server:/path/to/repo
            tokens = value.split()
            assert len(tokens) != 0
        except OSError:
            # Probably we are just packed. Find our entry in the packed refs file.
            # NOTE: We are not a symbolic ref if we are in a packed file, as these
            # are excluded explicitly.
            for sha, path in cls._iter_packed_refs(repo):
                if path != ref_path:
                    continue
                # sha will be used.
                tokens = sha, path
                break
            # END for each packed ref
        # END handle packed refs
        if tokens is None:
>           raise ValueError("Reference at %r does not exist" % ref_path)
E           ValueError: Reference at 'refs/heads/master' does not exist

.venv/lib/python3.10/site-packages/git/refs/symbolic.py:258: ValueError

(...)
=========================== short test summary info ============================
FAILED tests/pipelines/test_schema.py::TestSchema::test_get_schema_path_name - ValueError: Reference at 'refs/heads/master' does not exist
=================== 1 failed, 52 passed in 74.85s (0:01:14) ====================

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • [o] Documentation in docs is updated

Copilot AI review requested due to automatic review settings April 4, 2026 23:04
@codecov
codecov Bot commented Apr 4, 2026 •
edited
Loading
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.59%. Comparing base (5a777a5) to head (4aa72ab).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
nf_core/pipelines/list.py 90.62% 3 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI reviewed Apr 4, 2026
View reviewed changes
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses intermittent, parallel-test failures caused by shared access to Nextflow’s assets directory by isolating tests and avoiding unnecessary scanning of unrelated cached pipelines.

Changes:

  • Add a helper to consistently resolve the Nextflow assets directory (NXF_ASSETS / NXF_HOME / default).
  • Update get_local_wf() to directly inspect the expected cached workflow path instead of scanning all cached assets.
  • Update tests to use per-test temporary Nextflow asset directories and add coverage for broken git refs and “don’t scan unrelated repos”.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
nf_core/pipelines/list.py Centralizes Nextflow assets directory resolution and makes local-workflow lookup targeted to the requested workflow to avoid cross-test / cross-cache interference.
tests/pipelines/test_schema.py Isolates schema tests by setting NXF_HOME / NXF_ASSETS to temporary directories and restoring environment variables in tearDown().
tests/pipelines/test_list.py Adds regression tests for broken git refs and ensures get_local_wf() doesn’t scan unrelated cached repos.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/pipelines/test_list.py Resolved
@muffato
muffato commented Apr 4, 2026
Copy link
Copy Markdown
Member Author

@nf-core-bot changelog

muffato and others added 2 commits April 5, 2026 00:23
@muffato
Keep the hexsha exception scoped to this test
b9f43b6 
The test was configuring `type(repo.head.commit).hexsha = PropertyMock(...)`, which sets a `hexsha` property on the *Mock class* and can leak into other tests (global side-effect). Instead, assign a small stub object to `repo.head.commit` with a `hexsha` property that raises, or by setting `repo.head` / `repo.head.commit` to a dedicated mock class whose property you patch via `mock.patch.object`.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@muffato
Updated the changelog
becdc05
@muffato
muffato commented Apr 4, 2026
Copy link
Copy Markdown
Member Author

BTW I manually updated the changelog because the automated script couldn't run due to prettier check failing.

@muffato muffato added bug Something isn't working pipeline-testing labels Apr 4, 2026
mashehu
mashehu reviewed Apr 6, 2026
View reviewed changes
Comment thread nf_core/pipelines/list.py Resolved
@muffato
muffato force-pushed the test_schema_race_condition branch from ca5abca to b63ced0 Compare April 7, 2026 11:21
@muffato
muffato requested a review from mashehu April 8, 2026 10:56
@mashehu
mashehu commented Apr 8, 2026
Copy link
Copy Markdown
Member

BTW I manually updated the changelog because the automated script couldn't run due to prettier check failing.

Yes, sorry for that. I fixed the workflow (but it uses pull_request_target so we need to wait for a release to get this fixed), and then i fixed the changelog.md manually (by commenting the problematic table out). so it should work again in the other PRs

@muffato
muffato commented Apr 8, 2026
Copy link
Copy Markdown
Member Author

No pb.
I've merged dev to solve the merge conflict and CI is passing

mashehu
mashehu approved these changes Apr 8, 2026
View reviewed changes
Comment thread nf_core/pipelines/list.py Resolved
Comment thread nf_core/pipelines/list.py Resolved
Comment thread tests/pipelines/test_schema.py Resolved
muffato and others added 8 commits April 9, 2026 01:03
@muffato
Merge branch 'dev' into test_schema_race_condition
f3cfe04
@muffato @mashehu
More pathlib.Path
5d2d50c 
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
@muffato @mashehu
Bake the default value in the function call
217e11b 
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
@muffato @mashehu
To always use the same method for the same things
b2e5f82 
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
@muffato
These must be strings
f89084d
@muffato
os.environ.get is more frequent
0639e52
@muffato
Use the Path(a, b) syntax for consistency
f63d274
@muffato
The parent needs to be created too
4aa72ab
@muffato
muffato enabled auto-merge (squash) April 9, 2026 00:29
@muffato
muffato merged commit 4889c0c into dev Apr 9, 2026
112 checks passed
@muffato
muffato deleted the test_schema_race_condition branch April 9, 2026 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@mashehu mashehu mashehu approved these changes

Assignees

No one assigned

Labels

bug Something isn't working pipeline-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@muffato @mashehu
0