Skip to content

fix(test): gate the re.sub positional-flag DeprecationWarning test on Python 3.13 - #820

Merged
JarryShaw merged 1 commit into
mainfrom
fix-819-re-sub-deprecation-warning-gate
Sep 26, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix-819-re-sub-deprecation-warning-gate

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #819.

main was red on Python 3.11/3.12 (and their Engines legs): #813 added a
test asserting re.sub's positional-count DeprecationWarning
unconditionally, but that warning is a CPython 3.13 addition — 3.11/3.12
never raise it.

Inverted the assertion rather than skipping it, per the issue's preferred
fix: on 3.13+ the positional form must warn; below 3.13 it must not,
pinning the boundary as a tested fact rather than a comment (mirrors
#788's test_the_shim_switches_at_the_version_that_added_the_dunder).
Added a self-test proving the boundary constant is load-bearing, and fixed
a stale docstring cross-reference to the renamed method.

Verified on Python 3.11.15, 3.12.14, and 3.14.7 (repo venv) — all green,
and confirmed the wrong boundary reproduces the exact main failure on
3.12. Did not run make test (whole-suite OOM risk); ran the full
tests/vendor/ tree (85 tests) on 3.14 instead, all green.

… Python 3.13

main was red on Python 3.11/3.12 (and their Engines legs): the positional
form's DeprecationWarning is a CPython 3.13 addition, and the test added by
#813 asserted it unconditionally, with no version guard (#819).

- Invert the assertion instead of skipping it: on Python 3.13+ the
  positional form must warn; below 3.13 it must NOT warn, which is a true,
  testable fact rather than a comment -- so no leg goes dark. Factored the
  branching into `_assert_positional_flag_shape` so both the real test and
  its self-test exercise identical logic against a boundary parameter.
- Added a self-test proving the boundary constant is load-bearing: forcing
  the wrong branch on the running interpreter makes the assertion raise
  the exact "DeprecationWarning not triggered" symptom #819 hit, and the
  correct boundary holds regardless of which side of 3.13 is running.
- Fixed a stale cross-reference to the renamed test method in the module
  docstring.

Verified: `python -m unittest tests.vendor.test_re_sub_positional_flag_unit`
green on Python 3.11.15, 3.12.14, and 3.14.7 (repo venv, pcapkit resolved
from this tree); full `tests/vendor/` (85 tests) green on 3.14.

Closes #819
@JarryShaw JarryShaw added bug test Pull requests that add or correct tests (test: subject prefix) ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verified by me on three real interpreters, not by monkeypatch — the fail-before reproduces exactly and the fix holds:

              PRE-FIX (origin/main 55e1b756e)    POST-FIX (#820 ef7189350)
py3.11.15     FAILED (failures=1)                OK
py3.12.14     FAILED (failures=1)                OK
py3.14.7      OK                                 OK

Interpreters used: /home/jarryx/.local/share/mise/installs/python/3.11/bin/python3.11, /home/linuxbrew/.linuxbrew/bin/python3.12, and the repo venv at 3.14.7. The RuntimeDeprecationWarningTests class needs no pcapkit import, which is what makes running it on a bare interpreter possible.

It inverted rather than skipped, which is what I asked for and is the right call at this scale — a skipIf would have taken six legs dark (3.10, 3.11, 3.12 × Python and Engines), leaving coverage only on 3.13 and 3.14. The below-boundary branch asserts the absence of the warning rather than skipping:

if sys.version_info >= boundary:
    with self.assertWarns(DeprecationWarning): ...
else:
    with warnings.catch_warnings(record=True) as caught: ...
    self.assertFalse(any(issubclass(w.category, DeprecationWarning) for w in caught), ...)

POSITIONAL_COUNT_DEPRECATED_SINCE = (3, 13) is a class constant and the boundary is a parameter to the shared helper, which is what lets the self-test flip it without touching class state — a cleaner shape than I had in mind.

And its self-test finding is worth recording, because it corrects my own brief. I suggested proving the constant load-bearing by changing 13 → 12. The author did that literally and found the experiment is interpreter-dependent: on 3.11 and 3.14 the mutation causes no failure, since both sit strictly on one side of the 12/13 gap and the wrong boundary still agrees with real behaviour there. Only on 3.12 — the interpreter actually inside the gap — does it reproduce the original AssertionError: DeprecationWarning not triggered. So my suggested check would have produced a false all-clear on two of the three interpreters available. Its test_the_boundary_constant_is_load_bearing avoids that by forcing the wrong branch relative to whatever interpreter is running, and passes on all three.

It also caught a stale :meth: cross-reference to the old test name in the module docstring after the rename, by grepping for the old name — the kind of thing that survives a rename silently.

tests/vendor/ 85/85 on 3.14; RuntimeDeprecationWarningTests 5/5 on each of the three. pylint 10/10, isort clean, and the one mypy finding confirmed pre-existing on origin/main by diffing against the unchanged file. The make test box is left unticked with what was actually run described instead.

Labelled bug,test,ci,review: pending. Cross-review dispatching on a different model.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on the change: GOOD TO GO (haiku; author was sonnet). But I am NOT flipping the label yet, because this PR's own CI is incomplete — the reviewer withheld handover for exactly that reason, and it is the right call.

head ef7189350   ok=21  fail=0  inc=6
Python 3.10 / 3.11 / 3.12          COMPLETED SUCCESS
Engines Python 3.10 / 3.11 / 3.12  IN_PROGRESS      <- 3 of the 6 previously-red legs

This is the rule I adopted after clearing #813 on a partial run: review: good-to-go requires inc=0, not merely fail=0. Three of the six red legs have turned green, which is strong, but the Engines half has not reported. Flipping now would repeat the error this very PR exists to fix.

It found a 3.10 interpreter that neither the author nor I had, and ran five versions. /home/jarryx/.local/share/mise/installs/python/3.10.21/bin/python3.10. I reproduced the 3.10 leg myself: pre-fix FAILED (failures=1), post-fix OK. Its full matrix:

              base 55e1b756e     head ef7189350
3.10.21       FAILED             OK
3.11.15       FAILED             OK
3.12.14       FAILED             OK
3.13.15       OK                 OK      <- the boundary version itself, which neither of us had run
3.14.7        OK                 OK

The vacuity trap is defeated three ways, and the third is decisive. Repetition (3 consecutive runs in one process, all raising on 3.13/3.14, so registry caching is not suppressing re-emission); a deliberately poisoned __warningregistry__ for every re.sub(self.PATTERN…) line, still raising; and a counterfactual against the same branch with simplefilter('always') removed:

ambient filters                with simplefilter     without
-W ignore::DeprecationWarning  saw warning [T,T,T,T]  [F,F,F,F]    <- would pass vacuously
PYTHONWARNINGS=ignore          [T,T,T,T]              [F,F,F,F]

So simplefilter('always') is load-bearing and correctly placed.

It generalised my caveat into something reusable: a boundary mutated from B to B′ is detected only by interpreters v where min(B,B′) ≤ v < max(B,B′). So a fixed mutation probes at most |B−B′| minor versions — 13→12 fires on 3.12 alone, 13→14 on 3.13 alone. That is precisely why the PR's wrong_boundary built at ±1 from sys.version_info is load-bearing on every leg. And it proved the test is not just checking its own arithmetic: patching re.sub to silence the warning on 3.14, or force it on 3.10/3.12, makes both tests fail.

The rename is clean — git grep "would_warn" at head returns nothing repo-wide, self-tested both ways (a known-positive pattern returns 1 hit at head; the old name returns 2 at base, the def plus the docstring :meth:).

One thing worth fixing before merge, prose only: the new comment says the breakage hit "every leg below 3.13 (measured: Python 3.11, 3.12 …)", omitting 3.10 — which reproduces the partial-run scope error that #819's own correction retracted, in the file whose purpose is to stop that class of mistake. Two other wording nits: "actually_warns is the ground truth for THIS interpreter" overclaims (it is derived from the constant, so it is the asserted expectation; the logic is still sound because the helper measures), and the #: The module docstring at line 25 reference will drift on any docstring edit.

Noted, pre-existing, not actionable now: # noqa: intentional pre-fix shape suppresses nothing — not a valid flake8 payload, and pylint ignores # noqa — it documents intent, carried over from #813. And when the positional form becomes a TypeError as line 26 anticipates, assertWarns will receive it and these tests plus test_uncapping_count_only_matters_beyond_eight_matches go red; the repo's Compat Python 3.15 (scheduled) job would surface it.

Staying review: pending until inc=0. I will flip it the moment the three Engines legs report.

@JarryShaw

Copy link
Copy Markdown
Owner Author

CI is now complete and green — ok=27 fail=0 inc=0 — so the gate I was holding is satisfied and I am flipping to review: good-to-go.

All six legs that main fails on now pass here:

Python 3.10: SUCCESS          Engines Python 3.10: SUCCESS
Python 3.11: SUCCESS          Engines Python 3.11: SUCCESS
Python 3.12: SUCCESS          Engines Python 3.12: SUCCESS

That is the first time this session a good-to-go has been withheld and then granted on evidence rather than assumed — the rule from #813 doing its job. Together with the local five-interpreter matrix (3.10 through 3.14, pre-fix failing on 3.10/3.11/3.12 and post-fix passing everywhere, verified independently by me and by the reviewer), the change is as well evidenced as anything on the board.

This is the PR to merge first. main is red on six legs until it lands, and #817 — already review: good-to-go — inherits those same six failures and goes green once this is in.

The one prose item stands and does not block: the new comment enumerates "Python 3.11, 3.12" and omits 3.10, reproducing the partial-run scope error that #819's own correction retracted. Worth a one-line fix whenever convenient, in the file whose purpose is to prevent that class of mistake.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Scope correction, so merging this is not mistaken for making main green. This PR fixes #819 — 6 legs, Python 3.10/3.11/3.12 × (Unit, Engines). main at 3cbdf8999 is red on two independent defects, and the other one is #822: test_guess_version_reports_a_preface_with_no_frame_as_such, 10 legs, all five Pythons.

From main's own run 36201817798 — 3.12 carries both, 3.14 only the second:

3.12  FAILED tests/vendor/test_re_sub_positional_flag_unit.py::...::test_positional_flag_shape_would_warn   <- this PR
3.12  FAILED tests/protocols/application/test_http_unit.py::...::test_guess_version_reports_a_preface...    <- #822
3.14  FAILED tests/protocols/application/test_http_unit.py::...::test_guess_version_reports_a_preface...    <- #822

So main goes green when this and #822's fix have both landed, not on this alone. Still GOOD TO GO on its own scope — 27✅/0❌/0 incomplete on ef7189350, and this branch predates #814, so its green does not cover #822's interaction either way.

@JarryShaw
JarryShaw merged commit 319da66 into main Sep 26, 2026
31 checks passed
@JarryShaw
JarryShaw deleted the fix-819-re-sub-deprecation-warning-gate branch September 26, 2026 01:25
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci Pull requests that change CI or workflow configuration (ci: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: main is red on 3.11/3.12 — the re.sub DeprecationWarning test is ungated and CPython only warns from 3.13

1 participant