test: simplify consumer tests#565
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
de74a49 to
278e2a8
Compare
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
One important question, (regaridng logging deletion)
Other than that most things look sensible
| """ | ||
| Consumer tests: verify that downstream repos build successfully against this branch. | ||
|
|
||
| Via Python (requires ide_support to have been run first): |
There was a problem hiding this comment.
Nit: (can be done some other time)
If this is truly a requirement we could just do an assert that .venv_docs exists and otherwise exit early.
There was a problem hiding this comment.
We can add bazel support as well, but thats a separate PR. This was like that forever.
| ) | ||
| assert process.stdout is not None | ||
| for line in process.stdout: | ||
| print(line, end="") |
There was a problem hiding this comment.
Is this on purpose or left over from debugging?
There was a problem hiding this comment.
Can be removed in future PR's if that is the case.
There was a problem hiding this comment.
No that's for pytest with -s mode. As it's run from workflows as well.
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
For me this looks fine.
The two points I commented on are minor and can be removed in a future PR.
With this change we might can look at testing / expanding the tests soon.
Why
The old consumer test was a single monolithic test function that ran all repos and commands sequentially, using
os.chdir()to navigate between repo directories. This caused several problems:os.chdir()made the working directory a global side-effect, causing subtle failures when commands ran in the wrong directory.--repofilter option required comma-separated repo names and silently fell back to running all repos on typos, making targeted local runs error-prone.consumer_test.logviarich.Console, requiring a separatecatstep in CI to surface it. The CI workflow had asummarizejob to merge per-repo XML reports and artifacts.test_commandswas a separate field fromcommands, inconsistently populated — onlymodule_templateused it, with no tests actually wired up.What
Parametrized test structure — the single test function is replaced with a
@pytest.mark.parametrizetest over every combination of(repo, override_type, command). Each combination is now an independent test case that can pass, fail, or be xfailed individually. No more sequential blocking.No
os.chdir()— every subprocess call passes an explicitcwd=argument. The working directory never changes.localandremoteoverride types as first-class test dimensions — instead of running both override types inside a single test, each is a separate parametrized case. Remote tests check at collection time whether the commit is pushed and fail with a clear reason if not.Warnings forwarded to pytest —
WARNINGlines are forwarded viawarnings.warn()as the process runs; pytest collects them individually in its warnings summary. The process runs to completion before failing.Plain stdout instead of log file — output goes to
print()directly, visible with-s. Theconsumer_test.logfile and the CI artifact upload machinery are removed.-kreplaces--repo— repo and override type filtering uses standard pytest-ksyntax, consistent with the rest of the test suite.Changes
src/tests/test_consumer.py— full rewrite: parametrized tests, noos.chdir,local/remoteoverride split, warning forwarding,richdependency removed,get_current_git_hashfrom helper_lib used instead of a local reimplementationsrc/tests/conftest.py—--repooption removed;--disable-cachekept.github/workflows/consumer_test.yml— matrix updated to"<repo> and local"/"<repo> and remote"entries passed as-kfilter;summarizejob and artifact upload removed;--lockfile_mode=errorremoved fromide_supportcall.gitignore— added/.codexand/.claude