Remove dead process.env.CI skip guards from the integration tests - #30
Merged
Conversation
All four integration suites opened with:
if (process.env.CI) {
console.log('Skipping ... in CI environment');
return;
}
The guards never fired. The suites run via `docker compose run`, and the test
service passes only its explicit `environment:` list, so CI never reaches the
container -- `process.env.CI` is undefined there even on a GitHub runner. The
tests have always run in CI, which is what the guards were written to prevent.
Removing them rather than propagating CI: these four files are the whole
integration suite, so honouring the guards would leave CI with no integration
coverage at all. Note that it would not fail quietly -- Jest rejects a describe
block with no tests, so with CI set all four suites error out with
`Tests: 0 total`. Either way the guards no longer describe intended behaviour.
The flaky-browser-launch concern behind them is better addressed at the source;
the unbounded PlaywrightManager.close() that hung the keepalive hooks was fixed
in b8c907a.
Verified in the test container:
- CI=true now runs the integration suite: 40/40 pass (was 0 tests, 4 suites
failed, with the guards present)
- full suite: 136/136 pass
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.
Follow-up to #29, which flagged these.
All four integration suites opened with a guard intended to skip them in CI:
The guards never fired. The suites run via
docker compose run, and thetestservice indocker-compose.ymlpasses only its explicitenvironment:list, soCInever reaches the container —process.env.CIisundefinedinside it even on a GitHub runner. These tests have always run in CI, which is exactly what the guards were written to prevent.Why remove rather than propagate
CIThese four files are the entire integration suite, so honouring the guards would leave CI with no integration coverage.
One correction to what I wrote in #29: I said honouring them would "green CI while testing nothing." That's not right — Jest rejects a
describeblock containing no tests, so withCIset all four suites error out. Demonstrated in the container:Test SuitesTestsCI=trueCI=trueSo the guards would have failed loudly rather than quietly — smaller risk than I first described, but either way they no longer describe intended behaviour and are misleading to read.
The flaky-browser-launch concern behind them is better addressed at the source: the unbounded
PlaywrightManager.close()that was hanging the keepalive hooks got fixed in b8c907a.Verification
In the test container:
CI=true npm run test:integration:direct— 40/40 pass (was 0 tests / 4 failed suites with the guards)npm run test:all— 136/136 pass, 10/10 suitesNo production code touched; the diff is 24 deleted lines across the four test files.