diff --git a/docs/source/contributing/conventions/index.rst b/docs/source/contributing/conventions/index.rst index 562459cf18..b20777e2bd 100644 --- a/docs/source/contributing/conventions/index.rst +++ b/docs/source/contributing/conventions/index.rst @@ -13,9 +13,10 @@ House Conventions arisen. :ref:`sentinel-convention` governs :mod:`pcapkit.corekit`, :ref:`extension-header-subclassing` a protocol class hierarchy, :ref:`protocol-layer-placement` which subpackage a dissector belongs in, - :ref:`process` the repository rather than any of its code, and + :ref:`process` the repository rather than any of its code, :ref:`documentation` the prose itself -- on these pages and in the API - reference -- rather than any code at all. + reference -- rather than any code at all, and :ref:`test-coverage` what the + test suite has to prove. **A ruling that stays in its thread is a ruling that gets rediscovered.** So when a question is answered in a way the code cannot express on its own -- a @@ -35,3 +36,4 @@ House Conventions protocol-layer-placement process documentation + test-coverage diff --git a/docs/source/contributing/conventions/test-coverage.rst b/docs/source/contributing/conventions/test-coverage.rst new file mode 100644 index 0000000000..64bc010e45 --- /dev/null +++ b/docs/source/contributing/conventions/test-coverage.rst @@ -0,0 +1,184 @@ +.. _test-coverage: + +Testing a Change +---------------- + +The pages before this one rule on the library, the repository and the prose. This one +rules on what the test suite has to prove, and what a change has to bring to it. The +owner set the direction on :issue:`1203`, paraphrased here as on the other pages: every +round trip the library offers must pass and match exactly, the tests must cover all of +them, and the rules belong on a page rather than in review threads. :issue:`1202` tracks +extending round-trip coverage to the paths that do not have it yet. + +Round-Trip Invariants +~~~~~~~~~~~~~~~~~~~~~ + +**A round trip either reproduces its input exactly or is a recorded defect.** There is +no tolerance in between: a cycle that comes back with different octets is a bug, even +when both halves are internally consistent. + +Coverage is **enumerated from the registries rather than hand-picked**, so a code +registered tomorrow gets a case tomorrow. The two registry-driven suites that carry a +known-failure table keep their cases in a generator, beside the captures it also writes: + +.. list-table:: + :header-rows: 1 + :widths: 34 30 36 + + * - Suite + - Cases from + - Asserts + * - :file:`tests/protocols/test_option_roundtrip_unit.py` + - :file:`examples/generators/options.py` + - every option, chunk, parameter, message, frame and block code survives construct + -> parse -> construct with identical octets; known failures in + ``EXPECTED_FAILURES`` + * - :file:`tests/protocols/test_dispatch_registry_unit.py` + - :file:`examples/generators/dispatch.py` + - every ``__proto__`` entry dispatches to the class ``PINNED_TARGETS`` names; + known failures in ``KNOWN_DEGRADED`` (:issue:`496`) + +The rules those tables carry: + +* **Every registered code has a case.** ``test_every_registered_code_has_a_case`` + fails otherwise. The option suite's ``SKIP`` table is the only exit, and each of its + five entries states why a case would assert nothing: TCP's ``End_of_Option_List`` + and ``No_Operation`` are padding that ``_make_tcp_options`` drops, ``Multipath_TCP`` + is the envelope of the ``tcp-mptcp`` family and would duplicate one of its subtypes, + and ``PadN`` under HOPOPT and IPv6-Opts emits the same option-less header as + ``Pad1``. A code that fails is never skipped. +* **A failing case is an entry, never a changed argument.** An ``EXPECTED_FAILURES`` + entry records the status, a failure-detail fragment narrow enough to match one + site, and the defect: the ``file:line`` that causes it and the issue that tracks it. + Routing a case around a defect hides it: the generator's ``HIP_COPIES`` once put two + parameters in every HIP packet, so the unrepresentable length of a lone one + (:issue:`651`) was never constructed. Entries older than this page name the + ``file:line`` alone, and :issue:`1202` is where they gain their issue. + ``KNOWN_DEGRADED`` entries have a different shape -- a fragment and a reason -- and + its two record why the probe's placeholder payload cannot reach the target, not a + defect to fix, so the ``file:line``-and-issue rule does not apply to them. +* **A regression is fixed; a pre-existing defect is recorded.** A case that a change + breaks -- including a code it registers that does not close -- is fixed in that + change, not added to a table. A newly found defect that predates the change gets an + entry naming the ``file:line`` that causes it, as the failure message asks, and an + issue. +* **The tables are checked in both directions.** In the option suite, an entry whose + case starts passing turns ``test_round_trip_is_identity_or_a_recorded_gap`` red, and + one naming a case that no longer exists fails + ``test_expected_failures_name_real_cases``; the dispatch suite has the same pair. So + a fix deletes its entry in the same change. +* **The harness guards its own shape.** ``test_recorded_gaps_are_a_minority`` fails + once recorded failures reach half the option cases (the dispatch suite's + ``test_degraded_cases_are_a_small_minority`` draws the line at a quarter), since + that points at the harness rather than the library. Each case runs under + ``CASE_TIMEOUT``, 30 seconds in both suites, so a case that never finishes fails by + name instead of hanging the run. +* **A closed cycle is not proof of correctness.** A writer and a reader that share an + error round-trip perfectly: HIP's parameter padding (:issue:`651`) and IPv4's ``SID`` + width (:issue:`534`) both closed the cycle while wrong. So a round trip is paired with + an assertion against the specification -- the RFC's own lengths, or a known-good + packet's field values. + +A lossless path that is not registry-shaped gets a capture-driven case instead: +rebuild each layer with ``from_data`` and require the original octets back, as +:file:`tests/protocols/test_declared_length_roundtrip_runtime.py` does for the +declared lengths of a truncated capture. + +Raising Coverage +~~~~~~~~~~~~~~~~ + +**Every change raises coverage, and each new test is shown to fail without its fix.** +A test that also passes on the unfixed tree cannot tell the fix from its absence, so it +pins nothing. Before opening the pull request, take the fix out, run the new test, put +the fix back, and say in the pull request that you did: + +.. code-block:: shell + + git diff origin/main -- pcapkit/ > /tmp/fix.diff + git apply -R /tmp/fix.diff + python -m pytest tests//test__unit.py -q # must fail + git apply /tmp/fix.diff + python -m pytest tests//test__unit.py -q # must pass + +CI measures one leg, the ``test`` job's Python 3.14 cell in +:file:`.github/workflows/unit-tests.yml`, with branch coverage over :mod:`pcapkit` +configured by :file:`.github/coverage.toml`; :file:`.github/workflows/coverage-comment.yml` +posts the totals on the pull request. Locally, measure with ``coverage run`` rather than +a pytest plugin, which this project does not install, and narrow at report time -- +``[tool.coverage.run]``'s ``source`` in :file:`pyproject.toml` takes precedence over +``--include`` at run time: + +.. code-block:: shell + + python -m coverage run -m pytest tests//test__unit.py -q + python -m coverage report --include='pcapkit//.py' + +Running the Tests +~~~~~~~~~~~~~~~~~ + +* **One module per process, never the whole suite.** Check a change by running the + modules it touches, each as its own ``python -m pytest -q``, and leave the + whole tier to CI, which spreads it over workers with ``-n auto --dist load``. Every + purge-then-import leaves the previous generation of :mod:`pcapkit` behind as + garbage, and the whole suite in one plain-unittest process runs out of memory -- + :file:`util/run_unittest_leg.py` records the measurement. +* **Read the summary line, not the exit code.** pytest's last line + (``N passed, M failed``) and unittest's ``OK`` or ``FAILED (...)`` are the result; + an exit code read through a pipe or a wrapper is the pipe's or the wrapper's. +* **Keep to the tiers.** The unit tier must pass on a fresh clone with no generated + captures, installed as CI's ``test`` and ``unittest-ordering`` jobs install it, + ``pip install -e '.[test,DPKT,crypto,NGAP]'``; :file:`tests/integration/`, :file:`*_runtime.py` and + :file:`*_regression.py` are the fixture-dependent tier, run after ``make samples``. + A unit-tier module builds its own octets and never reads a generated capture -- + :file:`tests/_tiers.py` enforces that at collection and on every ``sample_path`` + call. +* **Import once per class.** A test class that needs a fresh :mod:`pcapkit` calls + ``reimport_once_per_class(self)`` from ``setUp``, in place of a per-test + ``purge_modules``, which re-imported the package before every test and dominated + the suite's time (:issue:`1065`). Both live in :file:`tests/_support.py`; pass + ``restore=True`` when the class must leave :data:`sys.modules` as it found it. +* **Check ordering under plain unittest.** :file:`tests/conftest.py` heals cross-module + import pollution after every test, so pytest alone cannot see it (:issue:`981`). + :file:`util/run_unittest_leg.py` runs one :file:`tests/` subdirectory, then the + root-level modules, in one plain-unittest process, which is what CI's + ``unittest-ordering`` job does per leg. Run it on the directory a change touches + when the change moves imports or module state: + + .. code-block:: shell + + python util/run_unittest_leg.py protocols/internet + python util/run_unittest_leg.py protocols --exclude protocols/internet + + The leg list lives in the workflow, not the script; :file:`tests/corekit/` and + :file:`tests/integration/` are deliberately not legs, for the reasons recorded + there. + +Adding a Protocol, Option or Dumper +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +What each must bring in tests before it merges, on top of the fail-without-fix proof +above: + +.. list-table:: + :header-rows: 1 + :widths: 18 82 + + * - Adding + - Must bring + * - an option-like code + - Registering it gives it a case automatically. Add arguments to the generator's + tables in :file:`examples/generators/options.py` where the ``_make_*`` defaults + are degenerate, and a ``Family`` to ``FAMILIES`` for a new registry. Its cycle + closes before it merges. + * - a protocol + - A unit-tier :file:`tests/protocols//test__unit.py` that builds its + own octets, asserts construct -> parse -> construct identity, and checks a parse + against the specification's field values. A ``__proto__`` registration also + needs its ``PINNED_TARGETS`` entry in :file:`examples/generators/dispatch.py`, + which ``test_every_case_has_a_pinned_target`` enforces. + * - a dumper + - Unit tests under :file:`tests/dumpkit/` asserting the output exactly, and a case + in :file:`tests/integration/test_output_formats.py` for the file that lands on + disk. That a format :mod:`pcapkit` also reads, such as PCAP, reproduces its + input byte for byte is the target :issue:`1202` tracks; nothing asserts it + yet. diff --git a/docs/source/contributing/testing.rst b/docs/source/contributing/testing.rst index 305b92ebdf..4f82319933 100644 --- a/docs/source/contributing/testing.rst +++ b/docs/source/contributing/testing.rst @@ -1,6 +1,10 @@ Testing ======= +What the suite has to prove, and what a change has to bring to it, is ruled on +:ref:`test-coverage`. Check a change module by module as that page describes; the +``make`` targets below run a whole tier. + Running the Tests ----------------- diff --git a/tests/project/test_conventions_doc_claims.py b/tests/project/test_conventions_doc_claims.py index a41a1f0e89..043363ab61 100644 --- a/tests/project/test_conventions_doc_claims.py +++ b/tests/project/test_conventions_doc_claims.py @@ -108,8 +108,9 @@ #: lived only in that thread. ``protocol-layer-placement`` carries the other ruling #: that thread settled -- which subpackage a dissector belongs in, decided by designed #: function rather than by encapsulation -- and sits beside the other class-hierarchy -#: page. Every anchor is the bare file stem, which is what :data:`PAGES` below depends -#: on. +#: page. ``test-coverage`` came last, carrying GitHub issue #1203's rules for what the +#: test suite has to prove. Every anchor is the bare file stem, which is what +#: :data:`PAGES` below depends on. ANCHORS = ( 'mint-criterion', 'sentinel-convention', @@ -118,6 +119,7 @@ 'protocol-layer-placement', 'process', 'documentation', + 'test-coverage', ) #: Each anchor's own file, one-to-one since the split -- there is no longer a single