Skip to content

The native overlay sample distribution is built by two copies of one helper, and they already disagree #420

Description

@kstonekuan

#419 added tests/test_packaging_manifest_parsing.py alongside the existing tests/test_packaging.py, and both now build the same sample distribution with their own copy of the same two helpers.

The copies already disagree:

tests/test_packaging.py:36                  def _write_example_distribution(
                                                temporary_directory: Path, *, include_record: bool = True
                                            ) -> tuple[Path, Path]
tests/test_packaging_manifest_parsing.py:22 def _write_example_distribution(temporary_directory: Path) -> Path

_example_record_path is duplicated verbatim in both.

This is drift at birth rather than drift over time, which is the useful thing about catching it now. Both copies write a sample_native_package with the same worker.py, the same 7.2 dist-info, and the same three RECORD rows. If that fixture ever changes (a second module, a different version, a RECORD with real hashes), one copy moves and the other keeps testing something else, and nothing fails.

What to do

Give the sample distribution one definition and have both test modules use it.

The decision this needs

Where shared test fixtures live in this repo, since there is more than one existing answer:

  • tests/conftest.py for a pytest fixture, which is the idiomatic home and gives both modules it for free.
  • A plain helper module, following packages/hflow-server/tests/ui_test_fixtures.py, which is the existing precedent for shared non-fixture builders.

Either is defensible. Pick one, say why in the PR, and prefer the one that leaves the call sites reading the way they do now: these tests want package_root in hand, not a fixture they have to unpack.

Note the two signatures differ for a reason, include_record=False is used by a test that checks the no-RECORD refusal, so the shared version has to keep that parameter. The caller that currently ignores the second tuple element should keep ignoring it, not have the return type changed to suit it.

Definition of done

  1. One definition of the sample distribution, used by both modules.
  2. include_record=False still reaches the test that needs it.
  3. No test changes behaviour: the same tests pass, and the mutation checks from test(packaging): cover native overlay manifest refusals #394 and test(packaging): cover native manifest parse refusals #419 still fail on their own refusals.
  4. _example_record_path likewise has one definition.

Not in scope

Merging the two test modules into one file. Splitting parse-stage refusals from apply-path behaviour is a reasonable division and this issue is about the fixture, not the file layout.

Validation

uv sync --locked --all-extras
uv run ruff check
uv run ruff format --check
uv run ty check
uv run pytest -q

These tests build a real native overlay, so they need a system C compiler (cc); see the setup section of CONTRIBUTING.md.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinggood first issueGood for newcomershelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions