Client side tests for analysis-runner by Alexander-Stuckey · Pull Request #758 · populationgenomics/analysis-runner

Alexander-Stuckey · 2026-07-05T22:36:59Z

Adding some client side tests for analysis-runner, before I make any changes to include Nextflow tower as a supported dispatch mode.

ALSO centralise dependency management into pyproject.toml

Add tests for both cli.py and cli_analysisrunner.py.
Minimal tests to ensure that jobs route correctly to cromwell.
Bump version of mypy and ruff, as they were quite outdated.
Centralise dependencies into pyproject.toml, delete setup.py etc, update build references.

Planning PR for the analysis-runner tw mode. No implementation yet — this commit only anchors the branch so reviewers can engage with the design described in the PR body before any code lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

MattWellie

Good start. You can push the pre-commit versions even higher - pre-commit in general is at 6.0.0+, mypy has a version v2.1.0.

The get_repository_specific_information method is probably running as part of this call, and will pass if this testing branch is currently pushed to github which is a bit meta. Probably worth adding those checks in, as it's something we'll be re-using in TW... unless we exclusively stick with named workflows, I can't remember if we came to a decision on this.

MattWellie · 2026-07-05T23:24:30Z

Confirming a few things from playing locally -

the dev dependencies aren't present, ideally requirements-dev.in should have pytest and pytest-mock listed
the dev dependencies are kinda... distributed at the moment in a couple of requirements files. Might be a nice upgrade to roll these into a extras_require section in the setup.py. The more modern approach would be to roll all the requirements, setup.py, and pyproject.toml into just a pyproject.toml... easier to manage too

in the tests:

the git checks are present and un-mocked, this is doing a real check against the local and remote git history, and will fail if an un-pushed commit it added locally - failing stdout example

The commit "fe7d23eab841cf77619ec6c05270f2a82489d012" was not found on the remote (Github). 
Please confirm if you want to proceed anyway. (y/n):

--

Feels bad raising these as issues and just... passing them over, lmk if you want to combine on this

MattWellie

LGTMatt - doesn't touch functional code, adds tests, updates/modernises requirements and packaging. What's not to love? Maybe a version bump and a second approval from software 👀

violetbrina

Thanks so much for your work adding tests to the analysis-runner, a much overdue addition for sure. I have some very minor comments, and I'm not sure if @dancoates or anyone else from @populationgenomics/software-team wants eyes on this as well.

But looking good.

violetbrina · 2026-07-12T23:12:24Z

+    mock_run_analysis_runner_from_args = mocker.patch(
+        'analysis_runner.cli.run_analysis_runner_from_args',
+    )
+    mock_run_cromwell_from_args = mocker.patch(
+        'analysis_runner.cli.run_cromwell_from_args',
+    )
+    mock_run_config_from_args = mocker.patch(
+        'analysis_runner.cli.run_config_from_args',
+    )


I'd probably make this a fixture given it's used in every test, just to reduce the duplication a little bit.

violetbrina · 2026-07-12T23:14:18Z

 .idea/
 .DS_Store
 .vscode/
+tmp_review_feedback.md


Probably don't need this in the git ignore if it's a temporary artifact? I assume we'd just not commit it in the first place.

Yeah I fixed this up in the other PR (I believe), but forgot to clean this up here.

violetbrina · 2026-07-12T23:16:40Z

+        lambda prompt: 'y',  # noqa: ARG005
+    )
+    run_analysis_runner(**args)
+    assert len(posted_requests) == 4  # noqa:PLR2004  # headers, json, timeout, url


I think I'd prefer an actual key or name check here rather than length which is a more fragile test.

violetbrina · 2026-07-12T23:18:17Z

+def _perform_version_check() -> None:
+    pass


Only runs at import time here, it would be better to use the autouse=True pytest fixture. See _no_outbound_requests.

violetbrina · 2026-07-12T23:19:40Z

+    "bump2version",
+    "pre-commit",
+    "pylint",
+    "ruff~=0.11",


I'm sure you just moved the existing requirements over which is great. But if we've upgraded to ruff 0.15 in the pre-commit, we should do the same in the dev dependencies as well.

Also, we should consider adding mypy here as well and also adding the same version in pre-commit. Just to prevent situations where local runs of ruff/mypy are different from the version the pre-commit tool uses.

Alexander-Stuckey · 2026-07-13T22:13:31Z

@violetbrina , I believe I've addressed everything, thanks for the feedback.

MattWellie · 2026-07-15T00:23:52Z

+    # via hail
+aiosignal==1.4.0
+    # via aiohttp
+analysis-runner[test] @ file:///opt/deps


Suggested change

analysis-runner[test] @ file:///opt/deps

analysis-runner[test]

This is breaking CI

I've made a side PR stacked on this branch - it un-nests the analysis-runner [dev] and [test] optional dependencies, and changes how the package is installed in CI.

It also rebuilds the requirements-dev file with uv instead of pip - it should be equivalent, but the file header is explicit about being created with UV Which we may not want

#760

Alexander-Stuckey and others added 2 commits May 20, 2026 15:27

Add client side tests, bump mypy and ruff

c2b699d

Alexander-Stuckey requested a review from MattWellie July 5, 2026 22:36

Alexander-Stuckey requested a review from a team as a code owner July 5, 2026 22:37

Alexander-Stuckey requested review from violetbrina and removed request for a team July 5, 2026 22:37

Merge branch 'main' into nextflow-tower-cli

a0a92ad

MattWellie approved these changes Jul 5, 2026

View reviewed changes

MattWellie self-requested a review July 5, 2026 23:18

Alexander-Stuckey added 2 commits July 6, 2026 12:52

Centralise requirements into pyproject.toml

cca98b1

Three more tests for repo checkout

5e2a9c8

MattWellie approved these changes Jul 6, 2026

View reviewed changes

Alexander-Stuckey mentioned this pull request Jul 7, 2026

Centralise deployment requirements #759

Closed

violetbrina requested changes Jul 12, 2026

View reviewed changes

lint

9bcd4cf

Alexander-Stuckey requested a review from violetbrina July 13, 2026 22:12

MattWellie reviewed Jul 15, 2026

View reviewed changes

	analysis-runner[test] @ file:///opt/deps
	analysis-runner[test]

Uh oh!

Conversation

Alexander-Stuckey commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattWellie left a comment

Choose a reason for hiding this comment

Uh oh!

MattWellie commented Jul 5, 2026

Uh oh!

MattWellie left a comment

Choose a reason for hiding this comment

Uh oh!

violetbrina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alexander-Stuckey commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Alexander-Stuckey commented Jul 5, 2026 •

edited

Loading