Repository navigation
CI wiring: cloud tests missing from the nightly, PR CI limited to main, and a test file that never runs #91
Description
Part of #90 (PR #61 catch-up). Rewritten 2026-09-01 after investigating the original claims — scope is substantially narrower than the first draft. The orphaned-dataset problem moved to #106.
What changed from the original issue
The first draft was framed around PR CI carrying production cloud credentials. That framing does not hold up:
- GitHub does not pass secrets to
pull_requestworkflows from forks, andci.ymlusespull_request, notpull_request_target. Only same-repo branch PRs get credentials, which requires write access. There is no path for an outside contributor to obtainTEST_USER_*credentials. - Running PR CI against prod is intentional. It gives a live read on whether the prod API is healthy, alongside whether NDI-python is. We are keeping it.
So the following are explicitly out of scope and should not be revived without new information: stripping credentials from ci.yml; pointing PR CI at dev; a destructive pytest marker; NDI_CLOUD_READONLY plus a conftest.py collection guard; tests/test_destructive_guard.py; splitting test-cloud-api.yml into prod-smoke and destructive-lifecycle jobs.
What remains is CI wiring: tests that do not run where they should.
1. Most live cloud tests never run in the nightly
.github/workflows/test-cloud-api.yml:44 runs exactly one file:
pytest tests/test_cloud_live.py -v --tb=shortThere are five other live cloud test files. They run only in ci.yml, which means they are only exercised when someone opens a PR — no coverage on a quiet day, and none of the prod monitoring signal that the nightly exists to provide.
| live file | mutating | read-only | PR CI | nightly |
|---|---|---|---|---|
test_cloud_live.py |
18 | 45 | yes | yes |
test_cloud_filehandler.py |
0 | 25 | yes | no |
test_cloud_read_ingested.py |
0 | 3 | yes | no |
test_cloud_matlab_license.py |
1 | 2 | yes | no |
test_cloud_hello_matlab.py |
0 | 1 | yes | no |
test_cloud_download_live.py |
— | — | no | no |
(test_cloud_api_datasets.py, test_cloud_api_documents.py and test_cloud_sync.py are mocked, need no credentials, and already run everywhere.)
Fix: widen the glob to tests/test_cloud_*.py.
This pulls test_cloud_matlab_license.py into the nightly, which requires NDI_CLOUD_TEST_USER_HAS_MATLAB_LICENSE to be set or tests/_matlab_license_guard.py raises a collection error by design. The two users need different values: user 2 is the account ci.yml already documents as having no MATLAB license ("false" — destructive tests run and clean up), while user 1's license state is not established, so it gets "true", under which the destructive tests skip themselves and the teardown never calls clearMatlabLicense. "true" is the conservative setting for an account whose state we have not confirmed.
2. CI does not run on PRs to non-main branches
ci.yml:3-7 and test-symmetry.yml:4-8 both restrict pull_request to branches: [main]. A PR targeting a feature branch gets no lint, no tests, no symmetry.
Worth being honest about the value: all 40 PRs in this repository's history target main. This gap has never once been hit. The fix is two lines and costs nothing today, but it is the lowest-value item here.
One consequence to record: because PR CI deliberately keeps hitting prod, widening the trigger increases prod write volume. Today it is bounded by "PRs to main only." If we start stacking PRs, each intermediate PR adds another round of cloud dataset churn, and the volume question becomes live again.
Fix: drop the branches: key from the pull_request trigger in both workflows.
3. concurrency needs to be job-level, not workflow-level
The original issue proposed adding concurrency with cancel-in-progress: true to ci.yml, noting it was "safe once no cloud state is in play." Since we are keeping cloud state in PR CI, that advice inverts — cancelling a superseded run strands whatever datasets it created mid-lifecycle, which is the same defect the original issue flagged in test-cloud-api.yml:11-13.
Fix: job-level concurrency on lint only, which holds no cloud state. The test job gets none.
4. test_cloud_download_live.py has never run anywhere
The file has no test_ functions. It is a standalone script built around section(), check(), run_test() and main(), driven by python tests/test_cloud_download_live.py --username ... --password .... Pytest collects zero tests from it.
531 lines that read as cloud coverage in the tree and in every directory listing, and have never executed in CI.
It is not a drop-in candidate for collection: it downloads a 743-document dataset, runs a full analysis pipeline, and hardcodes a prod dataset ID (668b0539f13096e04f1feccd) that would not resolve on dev.
Fix: rename it out of the test_* namespace so it stops advertising coverage it does not provide, and document it as the manual tool it is. Making it a real collected test is a separate decision with real runtime cost.
Out of scope
- Orphaned
NDI_PYTEST_TEMP_DATASETcleanup → Leftover NDI_PYTEST dataset cleanup: the in-test sweep is a silent no-op, and we don't yet know why #106 - Branch protection on
main— none exists, so no check here is enforceable. Needs org-level action, not a code change. - SHA-pinning actions (
actions/checkout@v4,actions/setup-python@v5are float tags) — reasonable hygiene, unrelated to everything above, and wants Dependabot or it rots. Should be its own issue.
Reference implementation on catchup/ndi-python-2026-08: commits c769e2a, db94df3, e119342, 8a8b43d. Note it was written against the original framing and implements much that is now out of scope.
Activity
[-]CI safety: PR CI carries cloud credentials, destructive cloud tests run against prod, and the dataset janitor deletes nothing[/-][+]CI wiring: cloud tests missing from the nightly, PR CI limited to `main`, and a test file that never runs[/+]