test(integ): let exhausted IAM throttling fail instead of skipping

[jam-jee]

Issue

The integ-test IAM throttling mitigation (merged in #6081) included a pytest_runtest_makereport hook that converted a SimulatePrincipalPolicy throttle surviving the adaptive retries into a skipped test. That masks a real regression: if the account is genuinely, persistently rate-limited, the affected tests silently drop out of the results instead of turning the build red. A skip reads as "not run," not "broken."

Fix

Keep the transient-throttling protection, drop the failure-hiding part:

• Kept — _configure_default_boto_retries (autouse, session-scoped). Adaptive retries (max_attempts=10, mode="adaptive") still smooth transient bursts, which is what fixes the common flaky-throttle case.
• Removed — the pytest_runtest_makereport skip conversion, plus its now-unused _is_simulate_policy_throttle helper and _THROTTLE_ERROR_CODES / _SIMULATE_OP constants.

Net effect: a throttle that exhausts the retry budget now fails the test loudly, so a persistent rate-limit regression stays visible.

Testing

• python -m py_compile passes.
• Verified the remaining imports (boto3, pytest, Config) are all still used; no orphaned references to the removed hook/helper/constants remain.

Type of change

• Bug fix (non-breaking; test-layer only)

Checklist

• I have read the CONTRIBUTING doc
• Changes are limited to a test conftest; no SDK source or public API is touched
• No new dependencies introduced

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

Claude finished @jam-jee's task in 31s —— View job

---

PR Review: let exhausted IAM throttling fail instead of skipping

• Read the diff (/tmp/pr.diff) and the resulting conftest.py
• Verify no orphaned references to removed hook/helper/constants
• Assess correctness, readability, and test-behavior impact

---

This looks good to merge. 👍

What the PR does, and why it's right

Removing the pytest_runtest_makereport skip-conversion is the correct call. Converting a persistent-throttle failure into a skip masks a real regression — a silent skip reads as "not run," not "broken," and would let a genuinely rate-limited account quietly drop tests from the results. Keeping the adaptive-retry fixture (max_attempts=10, mode="adaptive") still absorbs the common transient-burst case, which was the actual flakiness driver.

Verification

• No dangling references remain: _is_simulate_policy_throttle, _THROTTLE_ERROR_CODES, _SIMULATE_OP, and the pytest_runtest_makereport hook are all gone, and none are referenced elsewhere.
• Remaining imports (boto3, pytest, Config) are all still used by _configure_default_boto_retries.
• The module docstring was updated to match the new behavior — nice touch; it explicitly documents that exhausted-retry throttling now fails loudly.

Minor observations (non-blocking, no change required)

• The fixture reaches into a private attribute (boto3.DEFAULT_SESSION._session). That's a pre-existing pattern from test(integ): absorb iam:SimulatePrincipalPolicy throttling across integ suites #6081, not introduced here, so out of scope for this PR — just flagging it as a latent fragility if botocore ever changes internals.
• Behavior is now strictly "retry hard, then fail." If adaptive retries turn out to be insufficient under peak xdist concurrency, the mitigation is bumping max_attempts rather than re-adding the skip — which is the right direction.

Correctness, security, and performance are all fine here; the change is test-harness-only and touches no SDK source or public API.

[github-actions]

Claude finished @jam-jee's task in 1m 20s —— View job

---

PR Review: let exhausted IAM throttling fail instead of skipping

• Read the diff and resulting conftest.py
• Verify no orphaned references / unused variables
• Check docstring accuracy vs. implementation
• Post review

---

The core intent of this PR is right: converting a persistent-throttle failure into a skip masks a real regression, so removing pytest_runtest_makereport (and the now-unused _is_simulate_policy_throttle / _THROTTLE_ERROR_CODES / _SIMULATE_OP) is the correct call. Keeping the adaptive-retry fixture preserves the transient-burst protection that was the actual flakiness driver.

However, the deletion went one line too far and left two loose ends the diff introduced:

1. previous is now dead code, and the fixture teardown/docstring lie — sagemaker-serve/tests/integ/conftest.py:53-59

The diff removed everything after yield, which included not just the removed hook/helpers but also the env-var restore loop:

    previous = {
        "AWS_RETRY_MODE": os.environ.get("AWS_RETRY_MODE"),
        "AWS_MAX_ATTEMPTS": os.environ.get("AWS_MAX_ATTEMPTS"),
    }
    os.environ["AWS_RETRY_MODE"] = _RETRY_MODE
    os.environ["AWS_MAX_ATTEMPTS"] = _MAX_ATTEMPTS
    yield
    # (restore loop that consumed `previous` was deleted here)

Now previous is computed but never used, and the fixture's own docstring still says "Restores any pre-existing values on teardown." (line 52) — which is no longer true. This is out of scope for the PR's stated goal (the restore loop had nothing to do with the skip-conversion) and looks like collateral damage from deleting the whole post-yield block.

Two ways to fix, either is fine:

• Restore the teardown (recommended — keeps the fixture's documented contract): re-add the loop after yield.

      yield
      for key, value in previous.items():
          if value is None:
              os.environ.pop(key, None)
          else:
              os.environ[key] = value

• Or, if intentionally dropping restore (session-scoped, so it's low-impact), also delete the previous dict and update the docstring to stop claiming it restores anything — otherwise it's a dead variable that a linter (or reviewer) will flag.

2. Docstring refers to a fixture name that doesn't exist — sagemaker-serve/tests/integ/conftest.py:23

The rewritten module docstring documents the fixture as _configure_default_boto_retries, but the actual fixture is still named _configure_boto_adaptive_retries (line 49). Rename one to match the other so the docstring points at a real symbol.

Everything else is clean — no dangling references to the removed hook/helper/constants, remaining imports (os, pytest) are all used, and the "fail loudly" behavior change is sound. The private-attribute access noted in the prior review isn't present in this version (this fixture uses env vars, not boto3.DEFAULT_SESSION._session).

None of these are correctness-critical at runtime (session-scoped env-var leakage is benign in a test worker), but #1 in particular leaves the code internally inconsistent with its own docstring, and both are trivial to fix before merge.

[github-actions]

Claude finished @jam-jee's task in 59s —— View job

---

PR Review: let exhausted IAM throttling fail instead of skipping

• Read the diff (/tmp/pr.diff) and the resulting conftests
• Verify no orphaned references to removed hook/helper/constants
• Check the earlier-flagged loose ends (dead previous, docstring fixture name)
• Assess correctness, readability, and test-behavior impact

---

This is clean and correct to merge. 👍

The core change — dropping pytest_runtest_makereport and its supporting _is_simulate_policy_throttle helper plus the _THROTTLE_ERROR_CODES / _SIMULATE_OP constants — is the right call. Converting a persistent-throttle failure into a skip masks a real regression: a silent skip reads as "not run," not "broken," so a genuinely rate-limited account would quietly drop tests from the results. Keeping the autouse _configure_boto_adaptive_retries fixture (mode="adaptive", max_attempts=10) preserves the transient-burst protection that was the actual flakiness driver. Net effect is "retry hard, then fail loudly," which is what you want.

Verification (all three files)

• No dangling references: _is_simulate_policy_throttle, _THROTTLE_ERROR_CODES, _SIMULATE_OP, and pytest_runtest_makereport are gone and referenced nowhere else in the repo.
• Remaining imports are all still used (os, pytest in serve/train; boto3/pytest/Config etc. in mlops).
• Module/inline docstrings were updated to document the new fail-loudly behavior — consistent across all three conftests.

On the two issues flagged in the earlier review round — both are already resolved in the current diff:

• The env-var restore loop after yield is intact (sagemaker-serve/tests/integ/conftest.py:63-67, and the equivalents in train/mlops), so previous is still consumed and the fixture's "Restores any pre-existing values on teardown" docstring remains accurate. The earlier concern about previous becoming dead code does not apply to this version.
• The docstring references the fixture by its real name, _configure_boto_adaptive_retries (serve :24 / :51), not the _configure_default_boto_retries name that appeared only in the PR description. No mismatch in the code.

Minor, non-blocking (pre-existing, out of scope): sagemaker-mlops/tests/integ/conftest.py imports both os and import os as _os (lines 17-18) and uses each. That double alias predates this PR and isn't touched here — just noting it as latent cleanup, not something to address in this change.

Correctness, security, and performance are all fine — the change is test-harness-only and touches no SDK source or public API.