test: declare test runner dependencies - #388
Conversation
dosubot
Bot
added
size:S
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
==========================================
- Coverage 84.43% 84.42% -0.01%
==========================================
Files 104 104
Lines 6110 6107 -3
==========================================
- Hits 5159 5156 -3
Misses 951 951 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add coverage, mock, and pytest to the test extra and make CI rely on that advertised installation path. Closes deepmodeling#362 Coding-Agent: Codex Codex-Version: codex-cli 0.149.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
0d5ebc4 to
fb8c9e3
Compare
dosubot
Bot
added
size:XS
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Retracted: submitted without the maintainer's decision. Will re-review and let the maintainer choose the action.
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Right direction, and the archaeology backs it up: the ad-hoc pip install mock coverage pytest line predates the test extra by three PRs. #92 created test.yml with pip install -e . plus that line, back when pyproject.toml had only a docs extra; #102 appended pytest to it; #100 then created test = ['fakegaussian>=0.0.3'] and switched to pip install -e .[test] but left the ad-hoc line in place. So CI has always installed those three belt-and-braces, which is precisely why the missing declarations stayed invisible. There is no earlier decision here that this PR reverses.
All three additions are justified, including pytest, which is less obvious than it looks: tests/utils/test_bohrium_config.py matches unittest's default test*.py pattern, so discovery imports it, and its line 14 is import pytest. I simulated an environment without pytest and discovery produced a _FailedTest for that module; under CI's -f that aborts the run. So the entry is load-bearing even though the CI runner never executes the test itself.
Blocking on two things, both of which follow from the same decision: by deleting the belt-and-braces line, this PR makes the extra the sole contract, and the contract is not yet complete or consistent.
The contract still leans on a transitive dependency
tests/op/test_run_caly_model_devi.py does from ase import Atoms at line 14 and from ase.io import write at line 17, both at module scope, unguarded. unittest discovery reaches that file, so a missing ase is a collection error that takes down the run, not a skipped test. ase appears nowhere in pyproject.toml. It arrives only because fpop and cp2kdata (runtime deps) and fakegaussian (in this extra) each require it.
Worth stressing that this is genuinely a test-only dependency, not a runtime one, so the test extra is the right home: inside dpgen2/ there are no module-level ase imports at all. Every one is either inside a function (run_relax.py, run_caly_model_devi.py, distance_conf_filter.py) or inside the calypso_run_opt_str template string in prep_caly_input.py, which is written out as a standalone script rather than imported.
This is the same failure mode as #359, which you accepted as a bug and fixed in #385 by declaring the package. Before this PR, relying on the transitive path was merely untidy. After it, the extra is what CI trusts, and a minor release of fpop dropping ase would break the suite with a traceback naming a package this repository never declared. One line fixes it.
The repository's own setup instructions now contradict the workflow
.github/copilot-instructions.md#L44-L49 still says:
uv pip install mock coverage pytest fakegaussian ruff isortunder "Required for testing and linting". Four of those six are now in the extra. That file is both the contributor-facing setup document and the one the Copilot coding agent reads, and it is the only place in the repository that gives a concrete test-setup command, since README.md and docs/developer.md document none. So for a PR whose premise is that contributors and CI share one contract, this is the single document where that claim actually reaches a contributor, and it says something else. uv pip install -e .[test] ruff isort would settle it.
While you are there, that line is also the only record that ruff and isort are required dev tools, and neither has any declared extra. Not something to fix here, but worth knowing.
Not blocking
build (3.13) is red on this head, and I want to be clear that it is not yours. It failed on test_caly_evo_step with AssertionError: 'Failed' != 'Succeeded' after 198 tests. That test submits a live dflow workflow and polls query_status(); it touches no packaging code. The same test failed on feat/lmp-variable-precheck, on #405's branch (there on both 3.9 and 3.13), and on master itself in June 2026 on a commit that only bumped a Docker action. Master's 3.13 job is green on every recent push, and this branch's previous commit passed both jobs. It is a known intermittent failure and just needs a rerun before merge.
Two smaller notes, neither for this PR. tests/utils/test_bohrium_config.py is dead weight under both runners: test_handler_responses is a bare module-level def, so unittest collects nothing from it, and under pytest it fails, because the @pytest.mark.server decorators come from the pytest-mock-server plugin, which is not declared or installed. And dpgui sits in the runtime dependencies as well as both extras, redundant since #174 added it to the runtime list eight days after #172 put it in the extras.
| ] | ||
| test = [ | ||
| 'fakegaussian>=0.0.3', | ||
| 'coverage', |
There was a problem hiding this comment.
ase belongs in this list too. tests/op/test_run_caly_model_devi.py imports it at module scope (from ase import Atoms on line 14, from ase.io import write on line 17), unguarded, and unittest discovery reaches that file, so a missing ase is a collection error that takes down the whole run rather than skipping one test. It is declared nowhere in this file and arrives only because fpop, cp2kdata and fakegaussian each require it.
It is genuinely test-only, so this extra is the right place: dpgen2/ has no module-level ase import anywhere. They are all inside functions, or inside the calypso_run_opt_str template string that gets written out as a standalone script.
That makes this the same failure mode as #359, which was accepted and fixed in #385 by declaring the package. It mattered less while CI also ran pip install mock coverage pytest as a safety net; now that this extra is the only contract, an upstream release dropping ase would break the suite with a traceback naming a package this repository never declared.
| run: | | ||
| pip install -e .[test] | ||
| pip install mock coverage pytest | ||
| run: pip install -e .[test] |
There was a problem hiding this comment.
Good change, and it is the strongest possible check on the extra: CI now exercises the declared contract for real instead of papering over it. That is also why the ase gap I noted on pyproject.toml is worth closing in the same PR. Until this line changed, a wrong extra was harmless; from here on it is what CI trusts.
2 participants
Summary
coverage,mock, andpytestto thetestextrajsonpickleis a runtime import and is declared separately by PR #385 for issue #359.Tests
pyproject.tomlwithtomlliband verified the direct test dependencies.github/workflows/test.ymlwith PyYAMLgit diff --checkCloses #362
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh