Add publish ordering invariant tests (#71) - #86
Merged
leynos merged 15 commits intoJun 11, 2026
Merged
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughSplit the removed monolithic integration test into three focused test modules (preflight, workspace/config, publish ordering), added exported cargo command tuples and a ChangesIntegration test restructuring into functional modules
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 15 | ❌ 5❌ Failed checks (4 warnings, 1 inconclusive)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds comprehensive publish pipeline tests covering preflight behavior, workspace/config handling, and publish ordering invariants using both parametrized and property-based tests, plus a new helper for constructing deterministic N-crate dependency chains and a Hypothesis dev dependency. Flow diagram for new publish ordering test structureflowchart TD
conftest[tests_unit_publish_conftest_py]
helper[make_n_crate_chain]
preflight[tests_unit_publish_test_run_preflight_py]
ordering[tests_unit_publish_test_run_publish_ordering_py]
workspace[tests_unit_publish_test_run_workspace_config_py]
pipelines[publish_pipelines_dry_run_and_live]
conftest --> helper
helper --> preflight
helper --> ordering
helper --> workspace
preflight --> pipelines
ordering --> pipelines
workspace --> pipelines
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
issue-71-add-property-based-and-parametrised-tests-for-publish-ordering-invariants
branch
from
June 8, 2026 22:31
6c48969 to
6e5fdb2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Low Cohesiontests/unit/publish/test_run_integration.py: What lead to degradation?This module has at least 19 different responsibilities amongst its 20 functions, threshold = 4 Why does this problem occur?Cohesion is a measure of how well the elements in a file belong together. CodeScene measures cohesion using the LCOM4 metric (Lack of Cohesion Measure). With LCOM4, the functions inside a module are related if a) they access the same data members, or b) they call each other. High Cohesion is desirable as it means that all functions are related and likely to represent the same responsibility. Low Cohesion is problematic since it means that the module contains multiple behaviors. Low Cohesion leads to code that's harder to understand, requires more tests, and very often become a coordination magnet for developers. How to fix it?Look to modularize the code by splitting the file into more cohesive units; functions that belong together should still be located together. A common refactoring is EXTRACT CLASS. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# low_cohesion_example.js
var userLayer = connectUsers(myConnectionProperties);
-var chessEngine = startEngine(gameProperties);
+// [Refactoring: moved the data related to chess to a new chessGame.js module]
+// The module contains login related functionality that forms one behaviour: all
+// code is related since it either a) uses the same data, or b) calls the same functions.
export function login(newUser) {
val authenticated = userLayer.authenticate(newUser);
traceLoginFor(authenticated);
// ...some code...
}
-// playChess seems like a very unrelated responsibility.
-// Should it really be within the same module?
-
-export function playChess(loggedInUser) {
- var board = chessEngine.newBoard();
-
- return newGameOn(board, loggedInUser);
-}
+// [Refactoring: moved playChess to a new chessGame.js module
+// As a result of this refactoring, the module maintains a
+// single behavior where all code and data is related: high cohesion.] |
This comment was marked as resolved.
This comment was marked as resolved.
lodyai
Bot
force-pushed
the
issue-71-add-property-based-and-parametrised-tests-for-publish-ordering-invariants
branch
from
June 9, 2026 01:21
6e5fdb2 to
e2ec7d5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
June 9, 2026 22:06
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/publish/conftest.py`:
- Around line 171-179: Update the docstring for make_n_crate_chain to a full
numpy-style docstring: add a one-line summary followed by Parameters (root: Path
— root directory for crates; count: int — number of crates to create, must be
>=1) and Returns (tuple[WorkspaceCrate, ...] — tuple of crates wired as a linear
dependency chain, first crate has no dependencies, each subsequent crate depends
on the previous). Also add a short Examples section showing a minimal usage
snippet that calls make_n_crate_chain(root, 3) and describes the resulting
dependency relationships (crate_0 <- crate_1 <- crate_2). Ensure the
function-level description and types match the existing signature and that the
docstring is numpy-style.
In `@tests/unit/publish/test_run_preflight.py`:
- Around line 77-96: Many assertions in tests (e.g., those involving calls,
check_call, test_call, command, cwd, root) are bare; update each assert to
include a concise failure message. For example, where the diff asserts
membership or equality (like assert (("git","status","--porcelain"), root) in
calls, assert cwd == root, and assert command[2] == "--workspace"), change them
to use the form assert <condition>, "<brief message describing expected state>"
and do this consistently across tests in
tests/unit/publish/test_run_preflight.py and the related modules
test_run_workspace_config.py and test_run_publish_ordering.py so each assertion
has a clear message for failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3df01062-ed0c-4d87-aff1-152cd2fd9df1
📒 Files selected for processing (6)
pyproject.tomltests/unit/publish/conftest.pytests/unit/publish/test_run_integration.pytests/unit/publish/test_run_preflight.pytests/unit/publish/test_run_publish_ordering.pytests/unit/publish/test_run_workspace_config.py
💤 Files with no reviewable changes (1)
- tests/unit/publish/test_run_integration.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. tests/unit/publish/test_run_preflight.py Comment on lines +119 to +178 def test_run_includes_preflight_test_excludes(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
configured_excludes: tuple[str, ...],
expected_excludes: tuple[str, ...],
*,
unit_tests_only: bool,
) -> None:
"""Configured exclusions match the builder output and cargo invocation."""
configuration = make_config(
preflight=make_preflight_config(
test_exclude=configured_excludes,
unit_tests_only=unit_tests_only,
)
)
root, _workspace, calls = _setup_preflight_test(
monkeypatch, tmp_path, configuration
)
args, cwd = _extract_cargo_test_call(calls)
assert cwd == root, "cargo test should run in the workspace root"
arguments = list(args[2:])
assert arguments[0] == "--workspace", "cargo test should target the workspace"
include_all_targets = "--all-targets" in arguments
assert include_all_targets == (not configuration.preflight.unit_tests_only), (
"--all-targets should be present only outside unit-tests-only mode"
)
target_argument = next(
value for value in arguments if value.startswith("--target-dir=")
)
target_dir = Path(target_argument.split("=", 1)[1])
base_arguments = list(
publish_preflight._compose_preflight_arguments(
target_dir,
include_all_targets=include_all_targets,
)
)
options = publish_preflight._CargoPreflightOptions(
extra_args=tuple(base_arguments),
test_excludes=configured_excludes,
unit_tests_only=configuration.preflight.unit_tests_only,
)
rebuilt_arguments = publish_preflight._build_test_arguments(
list(base_arguments),
options,
)
assert rebuilt_arguments == arguments, (
"builder output should match the captured cargo invocation"
)
exclude_values = tuple(
arguments[index + 1]
for index, value in enumerate(arguments[:-1])
if value == "--exclude"
)
assert exclude_values == expected_excludes, (
"exclusions should be trimmed, deduplicated, and sorted"
)
if not expected_excludes:
assert "--exclude" not in arguments, (
"no --exclude flag should appear when there are no exclusions"
)❌ New issue: Excess Number of Function Arguments |
Owner
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of test_run_publish_ordering.py for single-crate coverage
echo "=== test_run_publish_ordering.py ==="
cat tests/unit/publish/test_run_publish_ordering.py
echo ""
echo "=== test_run_preflight.py ==="
cat tests/unit/publish/test_run_preflight.pyLength of output: 21771
Comment 1 — Single-crate coverage in Comment 2 — |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
lodyai
Bot
force-pushed
the
issue-71-add-property-based-and-parametrised-tests-for-publish-ordering-invariants
branch
from
June 10, 2026 00:18
a11c902 to
29a5327
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/publish/conftest.py`:
- Line 188: The docstring claims the parameter count "Must be at least 1" but
the implementation currently accepts 0; update the code so the behavior matches
the docstring by adding an explicit guard at the start of the crate-creation
function/fixture that checks the count parameter (if count < 1) and raises
ValueError("count must be >= 1"), or alternatively remove that lower-bound
statement from the docstring so they are consistent; ensure the check references
the count parameter used by the crate-creation routine.
In `@tests/unit/publish/test_run_preflight.py`:
- Around line 30-53: EXCLUDE_SCENARIOS is defined as a mutable list but should
be an immutable module-level constant; replace the surrounding square brackets
with parentheses to make EXCLUDE_SCENARIOS a tuple containing the pytest.param
entries (leave each pytest.param(...) item unchanged) so the constant is
immutable and follows the project's immutability guideline.
- Around line 113-124: EXCLUDE_MODE_SCENARIOS is created with a list
comprehension but is never mutated; change it to an immutable tuple by wrapping
the generator in tuple(...) instead of using a list literal so the module-level
constant becomes a tuple; update the expression that builds
EXCLUDE_MODE_SCENARIOS (which constructs _ExcludeScenario instances from
EXCLUDE_SCENARIOS and the unit_tests_only flag) to return a tuple to follow the
immutability guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 268ecbe4-3470-4e82-a1e5-4f0bebf5f8eb
📒 Files selected for processing (5)
tests/unit/publish/conftest.pytests/unit/publish/test_run_integration.pytests/unit/publish/test_run_preflight.pytests/unit/publish/test_run_publish_ordering.pytests/unit/publish/test_run_workspace_config.py
💤 Files with no reviewable changes (1)
- tests/unit/publish/test_run_integration.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (4 warnings, 1 inconclusive)
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== 1. threading/Lock in conftest.py ==="
rg -n "threading\|Lock\|_calls_lock" tests/unit/publish/conftest.py || echo "CLEAN - no threading"
echo ""
echo "=== 2. developers-guide.md - Hypothesis section ==="
fd -i "developers-guide.md" --exec grep -n "hypothesis\|Hypothesis\|property.based\|property-based\|make_n_crate_chain\|CallTrackingRunner" {} || echo "NOT FOUND"
echo ""
echo "=== 3. test_run_workspace_config.py - snapshot vs splitlines ==="
grep -n "splitlines\|in lines\|SnapshotAssertion\|snapshot\|_normalise_summary\|re\.sub\|normalised" tests/unit/publish/test_run_workspace_config.py
echo ""
echo "=== 4. Docstring coverage ==="
python3 - <<'EOF'
import ast
from pathlib import Path
import subprocess
result = subprocess.run(["fd", "preflight_test_utils"], capture_output=True, text=True)
extra_files = [f.strip() for f in result.stdout.strip().splitlines() if f.strip()]
files = [
"tests/unit/publish/conftest.py",
"tests/unit/publish/test_run_workspace_config.py",
"tests/unit/publish/test_run_publish_ordering.py",
"tests/unit/publish/test_run_preflight.py",
] + extra_files
total = 0
missing = 0
missing_items = []
for filepath in files:
p = Path(filepath)
if not p.exists():
print(f"NOT FOUND: {filepath}")
continue
tree = ast.parse(p.read_text())
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):
total += 1
has_doc = (
isinstance(node.body[0], ast.Expr)
and isinstance(node.body[0].value, ast.Constant)
and isinstance(node.body[0].value.value, str)
) if node.body else False
if not has_doc:
missing += 1
missing_items.append(f" {filepath}:{node.lineno} {type(node).__name__} {node.name}")
if total > 0:
print(f"\nTotal symbols: {total}")
print(f"Missing docstrings: {missing}")
print(f"Coverage: {(total-missing)/total*100:.1f}%")
if missing_items:
print("\nMissing:")
for item in missing_items:
print(item)
else:
print("All symbols documented.")
else:
print("No symbols found.")
EOF
echo ""
echo "=== 5. Snapshot .ambr files ==="
fd -e ambr . tests/unit/publish/ 2>/dev/null
echo ""
echo "=== 6. test_run_workspace_config.py full content ==="
cat tests/unit/publish/test_run_workspace_config.pyLength of output: 11535
The Code Duplication finding from the latest review comment is also clear — |
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Add a reusable `make_n_crate_chain` helper for publish tests so ordering invariants can be exercised across variable workspace sizes. Add Hypothesis to the development dependency group for the upcoming property-based publish ordering tests.
Parametrize the dry-run and live publish ordering tests across two, three, and five-crate dependency chains. Add Hypothesis properties that exercise the dry-run batching and live interleaving invariants across generated chain sizes from 2 to 10 crates.
Apply the formatter spacing required after replaying the publish ordering property tests onto the updated main branch.
Refactor test_run_integration.py into three focused modules: - test_run_workspace_config.py for root and configuration handling - test_run_publish_ordering.py for ordering and unpublished dependency behavior - test_run_preflight.py for preflight execution and related checks Preserve existing assertions and rename module boundaries without changing test logic.
Act on the publish ordering test review feedback: - Cover single-crate workspaces in the parametrised dry-run and live ordering tests, and lower the Hypothesis strategies to `min_value=1` so single-crate off-by-one issues are caught. - Exercise the exclude-normalisation scenarios in both `unit_tests_only` modes to confirm `_build_test_arguments` handles exclusions identically regardless of target narrowing. - Flesh out the `make_n_crate_chain` docstring with full numpy-style Parameters, Returns, and Examples sections. - Centralise the repeated cargo command tuples as shared constants in the publish conftest. - Give every assertion across the preflight, ordering, and workspace-config modules a concise failure message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeScene flagged `test_run_includes_preflight_test_excludes` for having five arguments (threshold four). Three of them were injected solely via `@pytest.mark.parametrize`. Wrap `configured_excludes`, `expected_excludes`, and `unit_tests_only` in a frozen `_ExcludeScenario` dataclass so the test signature shrinks to three parameters, keeping the parametrize ids and behaviour unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make `make_n_crate_chain` reject `count < 1` with a `ValueError` so the runtime behaviour matches its docstring's "must be at least 1" claim. Convert the `EXCLUDE_SCENARIOS` and `EXCLUDE_MODE_SCENARIOS` module-level constants from lists to tuples, since neither is mutated, to follow the project's immutability guideline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the inner runner/invoke closures, stub loaders, and the `_ExcludeScenario` dataclass across the new publish test modules so they reach 100% docstring coverage, clearing the >=80% gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a "Property-based testing" section covering the Hypothesis dev dependency and the publish suite's `@settings` convention, plus a "Publish test infrastructure" section tabulating the shared conftest helpers and constants, to the developers' guide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the per-substring assertions in `test_run_formats_plan_summary` with a syrupy snapshot of the full formatted summary so unintended formatting regressions are caught automatically. The snapshot now also covers the crate-count header, staging line, and README line that the substring checks ignored. Redact the two non-deterministic paths (the tmp_path workspace root and the randomly named staging directory) before comparing, so the snapshot is stable across machines and pytest runs, per the snapshot-hygiene guidance in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`publish.run` drives the injected command runner from sequential `for crate in plan.publishable` loops with no threads or executors, so `CallTrackingRunner` is only ever called single-threaded. The `threading.Lock` guarding its call log provided no safety guarantee, so remove it and the `threading` import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the per-substring assertions in `test_run_reports_no_publishable_crates` with a syrupy snapshot of the full formatted summary, matching the treatment already applied to its sibling `test_run_formats_plan_summary`. The snapshot also pins the crate-count line, staging line, and README line the substring checks ignored. Redact the non-deterministic workspace root and staging directory paths before comparing so the snapshot stays stable across machines and pytest runs, per the snapshot-hygiene guidance in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both `test_run_formats_plan_summary` and `test_run_reports_no_publishable_crates` carried an identical three-statement block redacting the workspace root and staging directory before the snapshot assertion. Extract it into a private `_normalise_summary` helper to remove the duplication CodeScene flagged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebasing the developers' guide additions onto main's restructuring left two doubled blank lines before the new "Property-based testing" and "Publish test infrastructure" sections. Collapse them so markdownlint's MD012 (no multiple consecutive blank lines) passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lodyai
Bot
force-pushed
the
issue-71-add-property-based-and-parametrised-tests-for-publish-ordering-invariants
branch
from
June 10, 2026 14:59
fef86c5 to
252152a
Compare
leynos
deleted the
issue-71-add-property-based-and-parametrised-tests-for-publish-ordering-invariants
branch
June 11, 2026 12:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch broadens publish ordering coverage for Issue #71 by checking the
live and dry-run pipelines across variable workspace sizes and generated crate
chain sizes.
Closes #71.
Review walkthrough
make_n_crate_chainfixture helper.Validation
make check-fmt: passedmake lint: passedmake test: passed, 481 testsmake typecheck: passedcoderabbit review --agent: passed with 0 findings after Phase 1coderabbit review --agent: passed with 0 findings after Phase 2Notes
The existing
make_dependency_chainhelper keeps its originalalpha,beta,and
gammacrate names so snapshot and index-missing tests retain their currentfixtures. The new
make_n_crate_chainhelper uses deterministiccrate_Nnames for arbitrary chain lengths.
Summary by Sourcery
Expand publish run test coverage for preflight checks, workspace configuration handling, and dry-run/live publish ordering invariants.
Enhancements:
Build:
Tests: