Skip to content

test: migrate credential-test gating to requires_credential marker - #9308

Merged
vicheey merged 1 commit into
developfrom
chore/credential-gate-to-marker
Oct 2, 2026
Merged

vicheey merged 1 commit into
developfrom
chore/credential-gate-to-marker

Conversation

@vicheey

@vicheey vicheey commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Which issue(s) does this change fix?

N/A

Why is this change necessary?

Credential-dependent integration tests were skipped by a stale proxy: SKIP_* = RUNNING_ON_CI and RUNNING_TEST_FOR_MASTER_ON_CI and not RUN_BY_CANARY. That gate dates from the AppVeyor era and reads a branch name to guess whether AWS credentials exist. CI now runs exclusively on GitHub Actions, so the branch-name proxy no longer describes anything real, and AppVeyor is retired. The right signal is simply "this test needs credentials" — a pytest marker — not an inference from the branch.

How does it address the issue?

It replaces the proxy gate with the @pytest.mark.requires_credential marker and lets each CI lane filter on it directly.

Part What
Marker Consolidate all five markers (requires_credential, tier1, tier1_extra, flaky, xdist_group) into pytest.ini's markers = block, and mark all 43 credential-gated test classes with requires_credential. These markers were all already in use and already registered elsewhere — requires_credential/tier1/tier1_extra via tests/conftest.py's pytest_configure, and flaky/xdist_group by the pytest-rerunfailures and pytest-xdist plugins — so this is a documentation/consolidation step that puts the whole marker vocabulary in the canonical config, not a first-time registration. tests/conftest.py is unchanged.
Flag removal Delete every credential @skipIf gate, the SKIP_CREDENTIAL_TESTS flag, its 25 alias imports, and a dead setUpClass guard. Compound gates keep their non-credential condition (IS_WINDOWS, IS_TARGETTED_PYTHON_VERSION). RUNNING_ON_CI now reads CI directly.
Lane filters The two no-credential PR lanes in build.yml (integ-all-other and the buildcmd catch-all) gain -m 'not requires_credential', so credential tests are deselected where no AWS credentials exist. integration-tests.yml already filtered correctly and is unchanged.
AppVeyor retirement Delete the 5 AppVeyor configs (appveyor-linux-binary.yml, appveyor-ubuntu.yml, appveyor-windows-al2023.yml, appveyor-windows-binary.yml, appveyor-windows.yml) and the installer AppVeyor cleanup steps.

The behaviour is identical to before: the no-credential lanes deselect exactly the set the credentialed canary lane selects — the marker just makes the decision at collection instead of guessing from the branch name.

What side effects does this change have?

One visible change in the no-credential PR lanes' pytest summary: credential tests are now deselected at collection rather than collected-then-skipped at runtime, so those lanes report fewer collected and fewer skipped tests. No test's run/skip outcome changes in any lane. The full tree still collects cleanly (2433 tests): pytest.ini sets filterwarnings = error, so any unregistered marker would raise PytestUnknownMarkWarning as a hard error — registering the five markers keeps that gate satisfied. (Note: --strict-markers is not enabled in this repo; filterwarnings = error is what enforces registration.)

How was this validated?

A two-part no-drop audit compared the base (develop, old @skipIf gates) against this branch, collecting under the real CI-lane environments. Both parts pass — the migration drops zero tests and runs the identical set in every lane.

1. Identical test universe. Full-tree pytest --collect-only yields the same total on both revisions; once a pre-existing parametrize-ordinal non-determinism (two unrelated, untouched parametrized tests whose _NN_<name> suffix shuffles between interpreter runs) is normalized, the two ID sets are byte-identical: 0 added, 0 dropped.

2. Per-lane run-set equivalence. For each lane, the base run set (collected minus what the old credential @skipIf would skip under that lane's env) equals the branch run set (collected after the -m marker filter):

Lane Env Base runs Branch runs Dropped New
build.yml buildcmd catch-all PR (no creds) 241 241 0 0
build.yml integ-all-other PR (no creds) 790 790 0 0
integration-tests.yml cloud-based-tests canary (creds) 119 119 0 0

The table counts the run set (what executes), which is identical before and after — that is the invariant this PR preserves. What does change is how pytest reports that set: for the buildcmd catch-all, both base and branch collect 272 and run 241, but base reaches 241 by collecting all 272 and skipping 31 credential tests at runtime, whereas the branch reaches 241 by deselecting those 31 at collection (272 collected, 31 deselected → 241 run). Same 241 executed; the 31 move from runtime-skip to collection-deselect.

3. No credential leak in no-credential lanes. Every PR (credential-free) lane selects zero requires_credential tests: the two lanes touching credential paths (integ-buildcmd-other: 31 present; integ-all-other: 114 present) deselect all of them via not requires_credential; the rest have no credential tests on their paths. The credential-heavy command groups (delete/deploy/package/publish/sync/traces/validate/logs) remain path-excluded from the PR matrix exactly as before. Non-credential grouped skips (SKIP_DOCKER_TESTS, SKIP_LMI_TESTS) were not migrated and still collect-then-skip, unchanged.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Review the generative AI contribution guidelines
  • Add input/output type hints to new functions/methods — N/A, no new functions
  • Write design document if needed (Do I need to write a design document?) — N/A, test-infra only
  • Write/update unit tests — N/A, no product code changed
  • Write/update integration tests — this change is the integration-test gating migration
  • Write/update functional tests if needed — N/A
  • make pr passes — validated via the full canary integration-tests run (24/24 green)
  • make update-reproducible-reqs if dependencies were changed — N/A, no dependency changes
  • Write documentation — N/A

…es_credential marker

Replace the legacy per-test credential gate -- SKIP_* = RUNNING_ON_CI and
RUNNING_TEST_FOR_MASTER_ON_CI and not RUN_BY_CANARY -- with the
@pytest.mark.requires_credential marker, now that CI runs exclusively on
GitHub Actions and AppVeyor is retired.

Changes:
- Register requires_credential + tiering markers in pytest.ini.
- Collapse 25 copies of the branch-name idiom; the branch term was a proven
  no-op (BY_CANARY is workflow-wide in the only master-triggered workflow).
  Drop the dead AppVeyor env reads; RUNNING_ON_CI now reads CI directly.
- Mark all 43 credential-gated classes with @pytest.mark.requires_credential,
  then delete every credential @skipIf gate, the SKIP_CREDENTIAL_TESTS flag,
  the alias imports, and the dead setUpClass runtime guard. Compound gates
  keep their non-credential condition (IS_WINDOWS, IS_TARGETTED_PYTHON_VERSION).
- Filter the two no-credential PR lanes in build.yml (integ-all-other and the
  buildcmd catch-all) with 'not requires_credential' so credential tests are
  deselected where no AWS credentials exist. integration-tests.yml already
  filters correctly (cloud-based-tests selects requires_credential; the
  credential-free suites exclude it).
- Retire the 5 appveyor-*.yml files and the installer appveyor cleanup steps.

Verified: full tree collects (2433) under --strict-markers; the no-credential
lanes deselect exactly the set the canary lane selects; no credential test runs
without credentials in any lane.
@vicheey
vicheey requested a review from a team as a code owner October 2, 2026 17:38
@vicheey

vicheey commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Integration test run: Integration Tests Integration Tests · aws/aws-sam-cli@5ee7ecc

@vicheey
vicheey merged commit 7aec621 into develop Oct 2, 2026
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants