test: expand integration coverage for CLI flags and orchestration#669
Closed
MatthewMckee4 wants to merge 1 commit intomainfrom
Closed
test: expand integration coverage for CLI flags and orchestration#669MatthewMckee4 wants to merge 1 commit intomainfrom
MatthewMckee4 wants to merge 1 commit intomainfrom
Conversation
Adds 32 new integration tests covering CLI flag combinations and orchestration behaviours that were either untested or only had a happy path. The additions are spread across basic, last_failed, configuration, and a new discovery/edge_cases module.
Merging this PR will not alter performance
|
Member
Author
|
Superseded by #670 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR closes a set of coverage gaps in the
crates/karva/tests/itintegration suite for CLI flags, orchestration behaviours, discovery edge cases, and configuration precedence that were either completely untested or only had a single happy-path assertion. It adds 32 new snapshot tests, all using inline snapshots andcommand_no_parallel()for determinism, and is intentionally scoped to areas not owned by the parallel fixtures/tags/snapshots/filterset coverage effort.What is now covered
Several CLI flags and their argument-parsing errors had no direct tests. The new tests in
crates/karva/tests/it/basic.rscover--color neverand the--color rainbowclap rejection,--no-cache,--no-progress(both with a passing suite and with a failure so that we can assert diagnostics still print),--retry 0(which turns out to be a no-op rather than an error),--retry Nagainst a test that always fails so retries exhaust, and the--max-fail=0rejection from theNonZeroU32type. The CLI parsing-error paths for--num-workers abc,--durations abc,--test-prefixwith no value,karva testx(the clap "did you meantest" suggestion), runningkarvawith no subcommand, and pointingkarva testat a nonexistent path all now have snapshot tests.The interactions between
--fail-fast,--no-fail-fast, and--max-failonly had isolated tests. Three new tests pin the precedence rules:--fail-fast --no-fail-fastuses clap'soverrides_withso the later flag wins,--no-fail-fast --fail-faststops after the first failure, and--max-fail=Nbeats--no-fail-fastregardless of order (documented in theSubTestCommand::into_optionscomment). There is also a new test showing that--num-workers 1matches the--no-paralleloutput exactly, and another showing that--dry-run --num-workers 4still only collects without spawning workers.The
--last-failedflag only had a happy-path test.last_failed.rsnow asserts that--last-failed --dry-runignores last-failed and prints every test (a subtle behaviour worth pinning),--last-failed -E test(~fail_a)intersects the last-failed set with the filter,--last-failed --max-fail=1stops scheduling after one failure in the rerun, and adding a brand new failing test after a run does not cause--last-failedto pick it up.For configuration,
configuration/mod.rsnow covers theKARVA_CONFIG_FILEenvironment variable (there was no test for it at all), confirms that--config-filewins when both env and CLI are set, and pins that akarva.tomlat the project root is still discovered whenkarva testis invoked from a subdirectory.The new
discovery/edge_cases.rsmodule fills a handful of discovery holes. It asserts that__pycache__directories and stray.pycfiles are ignored by discovery, that a package with__init__.pysits next to a standalonetest_*.pywithout either breaking the other, that.gitignoreexcludes directories by default and that--no-ignorerestores them, that a Python file containing only helpers (no test functions) is silently ignored, and that empty subdirectories alongside real test files cause no problems.Follow-ups
While probing the real behaviour to write these tests, a few surprising things turned up that I deliberately did not fix:
karva test --num-workers 0panics withindex out of bounds: the len is 0 but the index is 0atcrates/karva_runner/src/partition.rs:176. It should almost certainly be rejected by clap with a minimum value of 1, or short-circuited at the orchestration layer. No test is pinned for this because the panic output is noisy and the crash is unambiguously a bug.karva test --dry-run --last-failedignores--last-failedentirely and prints every discovered test, even after a previous run with known failures. I pinned this as observed behaviour inlast_failed_with_dry_run_shows_all_testsbut it is worth deciding whether dry-run should respect the last-failed filter so that users can preview exactly what a rerun would execute.--no-cacheis documented as "Disable reading the karva cache for test duration history," and indeed.karva_cache/run-*directories are still written after a--no-cacherun. The flag name reads like it disables the cache entirely, which is not what happens. The newtest_no_cache_flagonly pins that the run succeeds and does not attempt to assert cache-side effects either way.Test Plan
just test(856 tests, all passing)uvx prek run -a(all hooks passing)