Skip to content

test: reduce registry manifest test repetition#3146

Open
PascalThuet wants to merge 3 commits into
github:mainfrom
PascalThuet:chore/parallelize-integration-tests
Open

test: reduce registry manifest test repetition#3146
PascalThuet wants to merge 3 commits into
github:mainfrom
PascalThuet:chore/parallelize-integration-tests

Conversation

@PascalThuet

Copy link
Copy Markdown
Contributor

What

Reduce the expensive multi-install manifest contract from one CLI setup per safe-integration pair to two aggregate install orders: forward and reverse.

This keeps the pairwise manifest-disjoint assertion, but builds the manifests once per order instead of repeating the same setup for every pair.

Stack

Stacked on #3144. Because this fork cannot push the stack base branch to github/spec-kit (403), this draft PR targets main for upstream visibility and will include the #3144 diff until #3144 lands. After #3144 is merged, this PR diff should shrink to only tests/integrations/test_registry.py.

Validation

  • uv sync --extra test
  • uvx ruff check tests/integrations/test_registry.py
  • git diff --check
  • .venv/bin/python -m pytest tests/integrations/test_registry.py --durations=30 -q (1035 passed in 34.86s)
  • .venv/bin/python -m pytest tests/integrations --durations=30 -q before the final naming/comment cleanup (2500 passed, 1 skipped in 236.69s)

Comparison on the same machine:

  • Base stack PR: tests/integrations/test_registry.py collected 1223 tests and took 248.55s
  • This PR: tests/integrations/test_registry.py collects 1035 tests and took 34.86s

Disclosure

Prepared by Codex (model: GPT-5) on behalf of @PascalThuet.

Assisted-by: Codex (model: GPT-5, autonomous)
Assisted-by: Codex (model: GPT-5, autonomous)
@PascalThuet PascalThuet marked this pull request as ready for review June 24, 2026 08:05
@PascalThuet PascalThuet requested a review from mnriem as a code owner June 24, 2026 08:05
Add a >=2 precondition, explain why two install orders are tested
(manifests are order-independent; the orders only vary the init path),
and build the manifest map with a comprehension.
@PascalThuet

PascalThuet commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Pushed ca07e0e for the review feedback on test_safe_integrations_have_disjoint_manifests. The assertion is unchanged; this is clarity plus a guard.

  • Documented why both orders run. Manifest contents don't depend on install order: each safe integration writes only to its own disjoint directories and always records what it writes, so installing the whole set into one project gives the same per-integration manifests either way. forward/reverse doesn't vary the manifests; it just routes a different integration through init vs integration install (forward inits the first key, reverse the last). The old comment didn't say this and read as if order mattered.
  • Added a len(ordered_keys) >= 2 precondition. With fewer than two safe integrations the contract is vacuous and ordered_keys[0] would fail on an empty set, so a shrunken registry now fails here instead of passing silently.
  • Built the manifest map with a comprehension and pulled out integrations_dir. Cosmetic.

Validation: ruff check clean; pytest tests/integrations/test_registry.py -> 1035 passed in 6.48s.

(Disclosure: drafted and verified with an AI coding agent; reviewed by me.)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the multi-install-safe integration manifest contract test to reduce runtime by installing all safe integrations only twice (forward and reverse order) instead of repeating full CLI setup for every safe-pair combination. As part of the stacked changes, it also isolates integration-test HOME/XDG directories to prevent tests from reading/writing the real user home.

Changes:

  • Add an autouse pytest fixture to redirect HOME/USERPROFILE and XDG directories into each test’s tmp_path.
  • Replace the pairwise “init + install” manifest-disjointness loop with two aggregate install orders (forward + reverse) and then assert pairwise manifest disjointness from the resulting manifests.
Show a summary per file
File Description
tests/integrations/test_registry.py Refactors the manifest disjointness test to use two aggregate install orders and then check all safe-pairs for overlap.
tests/integrations/conftest.py Adds an autouse fixture to isolate HOME/XDG paths for integration tests.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment on lines +259 to +266
# Install every safe integration once into a single project, then assert
# pairwise manifest isolation. Each safe integration writes only to its
# own (disjoint) directories and always records what it writes, so a
# manifest's contents are independent of install order and of which other
# integrations are co-installed. The two parametrized orders therefore
# produce the same manifests; their purpose is to route a different
# integration through the `init` path versus `integration install`
# (forward installs the first key via init, reverse the last).

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback. If not applicable, please explain why

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants