Skip to content

test: one creds guard, and a net for the modules that forget it#648

Merged
ligon merged 1 commit into
developmentfrom
test/conftest-creds-skip
Jul 22, 2026
Merged

test: one creds guard, and a net for the modules that forget it#648
ligon merged 1 commit into
developmentfrom
test/conftest-creds-skip

Conversation

@ligon

@ligon ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Four PRs went red in a single day for the same non-reason — #625, #632, #641, #644. Each cost a diagnosis cycle to establish the failure had nothing to do with the change under review.

The pattern

A newly added test module builds real country data → DVC → S3. The CI unit-tests job runs deliberately data-free (LSMS_SKIP_AUTH=1, no secrets; only data-tests carries them), so it raises NoCredentialsError regardless of whether the logic is correct.

Most data-dependent tests already guard for this — 649 of them skipped cleanly in the same run that #625 errored in. The new modules simply lacked the guard.

#644 is the instructive one. It catches NoCredentialsError and re-raises it as:

Ethiopia/2011-12 cluster_features raised NoCredentialsError ... the geo file most likely spells its coordinate columns differently in this wave.

A confident, specific, and completely wrong diagnosis — pointing a reviewer at a data bug that does not exist.

Why it kept happening

There was no tests/conftest.py at all. So the same _aws_creds_available() helper had been copy-pasted into three modules, each noting centralising it was wanted "but not in a CI-red hotfix". This is that PR.

Adds the helper, a requires_s3 mark, and a pytest_runtest_makereport wrapper converting a missing-credentials failure into a skip — covering the setup phase too, since the usual shape is a module-scoped fixture building a country, where one error takes out the whole module at once.

It cannot hide a real failure

The conversion is conditioned on the environment, not the exception:

creds absent creds present
NoCredentialsError skipped failed
assert 1 == 2 failed failed

Verified in all four cells rather than assumed — a test silently skipped everywhere is worse than one red somewhere.

The subtlety that made this hard to verify (now documented in the file)

lsms_library/countries/.dvc/s3_creds is not tracked in git — it is written at import time by the auto-unlock. So a fresh git worktree looks credential-free until the first import silently creates it, after which the guard reads True. Only LSMS_SKIP_AUTH=1 reproduces CI’s actual state.

My first three attempts to verify this hook "failed" for precisely that reason, and the fourth only worked once I deleted the file and set the flag. That is recorded in the docstring so the next person does not lose the same hour.

Merge order

This should go in before the rebased country PRs — then their remaining red clears without per-module edits.

Four PRs in a single day went red for the same non-reason (#625, #632, #641,
#644): a newly added test module builds real country data, the CI `unit-tests`
job runs deliberately data-free (`LSMS_SKIP_AUTH=1`, no S3 secrets -- only
`data-tests` carries them), and the module lacked the skip guard its neighbours
have.  Each cost a diagnosis cycle to establish the failure had nothing to do
with the change under review.  #644's is the worst of them: it CATCHES
`NoCredentialsError` and re-raises it as a message about the geo file spelling
its coordinate columns differently, which is a confident and completely wrong
diagnosis.

There was no `tests/conftest.py` at all, which is why the same
`_aws_creds_available()` helper had been copy-pasted into three modules already,
each noting that centralising it was wanted "but not in a CI-red hotfix".

This adds it, plus `requires_s3`, plus a `pytest_runtest_makereport` wrapper
that converts a missing-credentials failure into a skip -- covering the `setup`
phase too, since the usual shape is a module-scoped fixture building a country,
where one error takes out the whole module.

## It cannot hide a real failure

The conversion is conditioned on the ENVIRONMENT, not on the exception:

  * credentials absent  -> convert to skip;
  * credentials present -> report the failure, unchanged.

A test silently skipped everywhere is worse than a test red somewhere, so this
was verified in both directions rather than assumed:

  creds absent   NoCredentials -> SKIPPED,  `assert 1 == 2` -> still FAILED
  creds present  NoCredentials -> FAILED,   `assert 1 == 2` -> still FAILED

## The subtlety that made verifying this hard, now documented in the file

`lsms_library/countries/.dvc/s3_creds` is NOT tracked in git -- it is written at
IMPORT TIME by the auto-unlock.  So a fresh `git worktree` looks credential-free
until the first import silently creates it, and the guard then reads True.  Only
`LSMS_SKIP_AUTH=1` reproduces CI's actual state.  My first three attempts to
verify this hook "failed" for exactly that reason.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon
ligon force-pushed the test/conftest-creds-skip branch from 05550ae to 243cb21 Compare July 21, 2026 22:14
@ligon
ligon merged commit 45aee17 into development Jul 22, 2026
10 checks passed
@ligon
ligon deleted the test/conftest-creds-skip branch July 22, 2026 02:50
ligon added a commit that referenced this pull request Jul 22, 2026
First CI run on this PR failed at collection with BOTH import spellings:

  from tests.conftest import requires_s3  -> ModuleNotFoundError: 'tests.tests'
  from conftest import requires_s3        -> ImportError: cannot import name
                                            'requires_s3' from 'conftest'
                                            (<repo root>/conftest.py)

The repo has conftest.py at BOTH the root and in tests/, and under the CI
rootdir the root one wins the bare name while 'tests' resolves relative to
tests/ itself.  This is a live trap for #648's helper: tests/conftest.py's own
docstring recommends 'from conftest import aws_creds_available', which no
module had yet exercised and which does not work in CI.

Load it by file path instead — import-mode agnostic, and still #648's single
source of truth rather than a fourth private copy of the creds check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
ligon added a commit that referenced this pull request Jul 22, 2026
…e ledger claim

Adversarial review of PR #643 returned FIX_FIRST.  The wiring and the diagnosis
survived it; three claims and two tests did not.  Every number below was
re-derived here, cold, in a wiped isolated LSMS_DATA_DIR, on both cores.

1. FALSE CLAIM, CORRECTED (the one with teeth).  The ledger and the PR body
   said "all 23 Niger tables build cold under LSMS_GRAIN_STRICT=1 -- 0 raises".
   Measured, #627's core, cold: **10 of 23 RAISE** -- cluster_features,
   household_roster, individual_education, shocks, assets, housing,
   food_security (2014-15), crop_production and plot_inputs (2011-12), plus
   household_characteristics as a cascade.  In the default mode all 23 build,
   10 of them emitting a GrainCollapseWarning; 0 cartesian warnings either way.
   What was actually established is narrower: #627's required-column
   RuntimeError no longer fires for Niger.  The grain conflicts are pre-existing
   on `development`, unrelated to the geo wiring, and remain open (GH #614/#637).
   A git-tracked ledger claiming "Niger is strict-clean" would have suppressed
   ten cells of real work -- the Albania "no shocks module" failure mode.
   Corrected in .coder/ledger/323-niger-config.md, in Niger/_/CONTENTS.org (new
   section, so the next reader of the country's own file cannot be misled), and
   in the PR body.

2. TWO TAUTOLOGICAL TESTS.  test_no_phantom_null_key_cluster and
   test_2011_12_is_at_cluster_grain pass with AND without `merge_how: left` --
   confirmed by dropping only that key: the built table is identical, because
   the (t, v) collapse goes through groupby, which drops NaN-key rows anyway.
   The PR body's "checked ... on the built table" was wrong.  What `left`
   actually buys is the elimination of the `nan_key_rows: 1` grain report -- a
   GrainCollapseWarning, fatal under LSMS_GRAIN_STRICT=1.  Added
   test_2011_12_emits_no_nan_key_grain_warning, which asserts exactly that and
   fails when the key is dropped; the two invariant tests now say in their
   docstrings that they do not discriminate and which test does.

3. THE END-TO-END TESTS DISARMED THEMSELVES.  The fixture was
   `except Exception: pytest.skip(...)`.  Measured: the old file, reverted
   config, #627's core -> 3 failed, 3 passed, 3 SKIPPED; under
   LSMS_GRAIN_STRICT=1 at HEAD -> 6 passed, 3 SKIPPED.  The three tests that
   touch real numbers went green-by-skip on the very regression they exist to
   catch.  The fixture now catches nothing: tests/conftest.py's
   missing-credentials net (PR #648, merged in here) covers the data-free CI
   job, and a GrainCollapseError/RuntimeError fails the file.  The module drops
   LSMS_GRAIN_STRICT for its own duration and asserts the strict *condition*
   directly instead, so it neither skips nor trips over Niger 2014-15's
   unrelated pre-existing conflict.

Also: `<= 1` rather than `== 1` on the offsets file's trailing null-key row (a
WB release artifact, not something to fail a test over), and a note on
test_merge_how_is_left that the key is inert until #627 lands.

DISCRIMINATION, re-measured (10 tests, each row a separate cold run):

  HEAD config              / development core  ->  9 passed, 1 skipped
  HEAD config              / #627 core         -> 10 passed (also under strict)
  2011-12 @ 3488b79^      / development core  ->  4 failed, 5 passed, 1 skipped
  2011-12 @ 3488b79^      / #627 core         ->  3 failed, 4 errors, 3 passed
  HEAD minus merge_how     / #627 core         ->  2 failed, 8 passed

The reviewer's count of 4 (not the ledger's 3) is right: the fourth is the
end-to-end test_2011_12_clusters_all_have_coordinates.

Still no change under lsms_library/*.py; Niger/_/niger.py untouched; no
`aggregation:` key; no wave `data_info.yml` edited (a comment-only edit there
would invalidate the wave's cache hash for nothing).

Refs #323, #515, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
ligon added a commit that referenced this pull request Jul 22, 2026
…tests

Dropping the blanket `except Exception: skip` left the data-dependent tests
resting entirely on tests/conftest.py's missing-credentials NET, which fires
only on a `NoCredentialsError`-shaped failure.  If the data-free CI job fails
some other way -- a RuntimeError out of `data_access` under LSMS_SKIP_AUTH=1,
say -- the net does not catch it and the job goes red for an environmental
reason.  So the six data-dependent tests now carry `requires_s3` explicitly,
which is what the other data-touching modules do.

`from conftest import requires_s3` does NOT work: the repo has a root-level
`conftest.py` and it wins on sys.path, so the plain import raises ImportError
(measured).  Rather than grow yet another private `_aws_creds_available` copy
-- the duplication PR #648 exists to end -- `tests/conftest.py` is loaded by
path under its own module name.

Unchanged: 10 passed on #627's core, 9 passed / 1 skipped on `development`'s,
and dropping only `merge_how: left` still fails exactly two.

Refs #323, #627, #648.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
ligon added a commit that referenced this pull request Jul 22, 2026
fix(#648): the documented conftest import does not work — use a marker instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant