Declare explicit installer binaries and add workflow contract tests#196
Merged
Declare explicit installer binaries and add workflow contract tests#196
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
SummaryThis pull request aligns the installer package's Cargo binary targets with the rolling-release workflow contract by declaring explicit workflow-facing binaries, centralising cargo metadata helpers, and adding tests and documentation to validate the contract. Changes
RationalePrevent CI/workflow build and test failures by ensuring packaging workflows use explicit, workflow-facing Cargo binary targets (rather than source-derived fallback names). The changes are additive and limited to build/packaging metadata, tests and developer documentation; there are no runtime or user-facing behavioural changes. Design contextThis implements the release-workflow contract described in the execplan docs/execplans/4-3-1-release-workflow-for-whitaker-installer.md (Status: COMPLETE) and supports the rolling-release workflow defined there. WalkthroughDisable Cargo autobin discovery for the installer crate, declare explicit [[bin]] targets, add cargo-metadata helper functions and unit tests, add a workflow contract test verifying declared installer binaries, and document the installer helper binaries in the developer guide. Changes
Sequence Diagram(s)Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAligns the installer crate’s explicit Cargo binary targets with the rolling-release workflow expectations and adds workflow tests/helpers to assert the packaging binaries match the workflow contract and do not rely on filename-derived fallback names. Sequence diagram for workflow test validating installer packaging binariessequenceDiagram
actor Pytest
participant RollingReleaseWorkflowTests as RollingReleaseWorkflowTests
participant WorkflowTestHelpers as WorkflowTestHelpers
participant Cargo as CargoCommand
participant CargoMetadata as CargoMetadata
Pytest->>RollingReleaseWorkflowTests: test_installer_packaging_bins_match_release_workflow_contract()
RollingReleaseWorkflowTests->>WorkflowTestHelpers: package_binary_target_names(whitaker_installer)
WorkflowTestHelpers->>Cargo: run cargo metadata for package whitaker_installer
Cargo-->>WorkflowTestHelpers: metadata JSON
WorkflowTestHelpers->>CargoMetadata: load_metadata()
CargoMetadata-->>WorkflowTestHelpers: bin_targets [names]
WorkflowTestHelpers-->>RollingReleaseWorkflowTests: list of bin target names
RollingReleaseWorkflowTests->>RollingReleaseWorkflowTests: assert whitaker_package_lints in bin target names
RollingReleaseWorkflowTests->>RollingReleaseWorkflowTests: assert whitaker_package_installer in bin target names
RollingReleaseWorkflowTests->>RollingReleaseWorkflowTests: assert package_lints not in bin target names
RollingReleaseWorkflowTests->>RollingReleaseWorkflowTests: assert package_installer_bin not in bin target names
RollingReleaseWorkflowTests-->>Pytest: test result
Class diagram for installer crate binaries and workflow testsclassDiagram
class InstallerCrate {
+string name
+bool autobins
+list~BinTarget~ bin_targets
}
class BinTarget {
+string name
+string path
}
class CargoToml {
+string path
+InstallerCrate installer_crate
+void load()
+void save()
}
class CargoMetadata {
+string package_name
+list~BinTarget~ bin_targets
+void load_metadata()
}
class WorkflowTestHelpers {
+list~string~ package_binary_target_names(package_name)
}
class RollingReleaseWorkflowTests {
+void test_installer_packaging_bins_match_release_workflow_contract()
}
InstallerCrate "1" o-- "many" BinTarget : declares
CargoToml "1" o-- "1" InstallerCrate : configures
CargoMetadata "1" o-- "many" BinTarget : describes
WorkflowTestHelpers ..> CargoMetadata : uses
RollingReleaseWorkflowTests ..> WorkflowTestHelpers : calls
InstallerCrate : +BinTarget whitaker_installer
InstallerCrate : +BinTarget whitaker_package_lints
InstallerCrate : +BinTarget whitaker_package_installer
BinTarget : +string whitaker_installer
BinTarget : +string whitaker_package_lints
BinTarget : +string whitaker_package_installer
Flow diagram for validating Cargo binary targets for whitaker installerflowchart TD
A[Start validation for package whitaker_installer] --> B[Run cargo metadata for package whitaker_installer]
B --> C[Extract bin targets from metadata]
C --> D{Do bin targets include
whitaker_installer,
whitaker_package_lints,
whitaker_package_installer?}
D -- No --> E[Fail: expected workflow-facing binaries missing]
D -- Yes --> F{Do bin targets include
fallback names
package_lints or
package_installer_bin?}
F -- Yes --> G[Fail: fallback filename-derived binaries still present]
F -- No --> H[Pass: installer packaging binaries match workflow contract]
E --> I[End]
G --> I[End]
H --> I[End]
File-Level Changes
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.
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/workflows/test_rolling_release_workflow.py`:
- Around line 69-93: The test
test_installer_packaging_bins_match_release_workflow_contract is missing an
assertion for the dependency packaging binary used by the workflows; update that
test (which calls package_binary_target_names("whitaker-installer")) to include
assert "whitaker-package-dependency-binary" in binary_targets with a clear
failure message (e.g., stating that release and rolling-release workflows build
--bin whitaker-package-dependency-binary but the installer package does not
declare it) so the test covers both release and rolling-release workflow
contracts.
In `@tests/workflows/workflow_test_helpers.py`:
- Around line 46-53: The docstring on the private helper function
_cargo_metadata is currently multi-line with a Returns section; replace it with
a single-line summary docstring: change the block docstring for _cargo_metadata
to a one-line string describing its purpose (e.g., "Return parsed workspace
metadata from `cargo metadata --no-deps`.") so the function follows the
private-function docstring guideline and numpy style expectations.
- Around line 54-55: The test helper _cargo_metadata currently calls "cargo" by
name and suppresses S607; resolve the cargo executable with
shutil.which("cargo"), assert it's found (e.g., assert cargo_executable is not
None), remove the S607 noqa, and pass the resolved cargo_executable into
subprocess.run (replacing ["cargo", ...] with [cargo_executable, ...]) while
keeping the existing cwd, capture_output, text, timeout, and check settings.
🪄 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
Run ID: f029fb6e-c470-4c03-8942-4f6083ec47b4
📒 Files selected for processing (3)
installer/Cargo.tomltests/workflows/test_rolling_release_workflow.pytests/workflows/workflow_test_helpers.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24628
Overall Comment 1 — shared Overall Comment 2 — exact equality check for binary kind target.get("kind") == ["bin"]This correctly rejects any target whose Individual Comment 1 — assert assert "whitaker-installer" in binary_targets, (
"installer package must expose primary whitaker-installer binary target "
"to satisfy release workflow contract"
)—before asserting the two packaging helpers and the two negative cases. None of the three requirements warranted annulment: no en-GB-oxendict spelling conflicts are present (the code already uses "artefact"), and none of the changes increase cyclomatic complexity. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Added package_binary_target_names helper to query Cargo bin targets - Added test_installer_packaging_bins_match_release_workflow_contract to ensure workflow build targets match installer Cargo.toml declared bins - Updated installer/Cargo.toml to declare specific binary targets This ensures the CI workflow's built binaries align with Cargo package declarations. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…r test Refactor multiple test helper functions to create dedicated helpers for accessing cargo metadata and package-specific metadata, reducing duplication and improving clarity in test code. Add an assertion in test_rolling_release_workflow ensuring the 'whitaker-installer' binary target is present, enforcing release workflow contract. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…h assertion - Added assertion for 'whitaker-package-dependency-binary' in rolling release workflow test to ensure installer package declares this binary. - Updated _cargo_metadata helper to resolve 'cargo' executable path explicitly and assert presence, improving test reliability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
force-pushed
the
fix-whitaker-package-lints-build-y30990
branch
from
df6ba77 to
2ddb956
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/workflows/workflow_test_helpers.py`:
- Around line 51-52: Update the inline suppression on the subprocess.run call
(the `completed = subprocess.run(...)` line) to replace the free-form `# noqa:
S603 # FIXME: uses trusted test-only tool` with a repository-compliant `# noqa:
S603 # FIXME: <TICKET-URL or ID> - uses trusted test-only tool`, i.e., add a
real ticket link or ID after `FIXME:` so the narrow disable includes a
justification plus a traceable ticket reference.
🪄 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
Run ID: f325dfab-c2c5-4bb8-ae86-a631cf2bc971
📒 Files selected for processing (3)
installer/Cargo.tomltests/workflows/test_rolling_release_workflow.pytests/workflows/workflow_test_helpers.py
* docs(installer,workflows): add detailed docs and tests for installer binaries workflow contract - Add extensive documentation in developers-guide.md on installer release-helper binaries, explaining explicit binary target declarations. - Introduce Python unit tests for workflow test helpers validating Cargo metadata resolution and workflow contract adherence. - Update small rust test and add comments to clarify maintainer guidance for adding installer packaging binaries. - Add no functional changes, focus on improving maintainability and preventing workflow breakage through testing and documentation. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * test(workflows): parametrize tests for workspace package metadata helper - Combined two separate tests into one parametrized test to check invalid metadata. - Added test cases for non-list packages and missing workspace package. - Improved test readability and reduced code duplication. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): update test helpers to use patch.object and enhance mocks - Replace patch() with patch.object() for mocking shutil.which and subprocess.run - Simplify TOML metadata test to use more general path matching - Improve workspace package metadata tests for better error handling - Streamline patch context usage and reduce nested with statements - Enhance test reliability and maintainability by refining mocks and asserts Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> --------- Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai Have the following now been resolved? ❌ Failed checks (2 warnings)
|
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: Length of output: 59549
Testing A dedicated test file
All three Developer Documentation
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/workflows/test_workflow_test_helpers.py`:
- Line 89: Replace bare asserts that compare types with message-bearing
assertions in tests/workflows/test_workflow_test_helpers.py: for the assertion
using type(parsed).__name__ == expected_type_name, change it to include an
explanatory message (e.g., assert type(parsed).__name__ == expected_type_name,
f"parsed type {type(parsed).__name__} != expected {expected_type_name}"). Do the
same for the other bare asserts referenced (the ones comparing parsed/expected
values at the other locations), ensuring each assert uses the involved variable
names (parsed, expected_type_name, etc.) in the message so CI failures are
diagnostic.
- Line 140: Update the parameter type annotation on the test helper signature
that currently reads "self, return_value: dict, expected_match: str" to use a
precise modern-style mapping type: change "return_value: dict" to "return_value:
dict[str, object]"; keep the parameter name and the rest of the signature
unchanged so the test contract remains explicit.
🪄 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
Run ID: 58151fed-1f5b-4259-ab3d-f0e53350f80f
📒 Files selected for processing (4)
docs/developers-guide.mdinstaller/tests/behaviour_docs.rstests/workflows/test_workflow_test_helpers.pytests/workflows/workflow_test_helpers.py
…pers Enhanced assertions in tests/workflows/test_workflow_test_helpers.py by adding informative failure messages to aid debugging when test assertions fail. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
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
Changes
package_binary_target_names(package_name)and addedtest_installer_packaging_bins_match_release_workflow_contractto ensurepackaging bins align with the workflow contract:
whitaker-package-lintsandwhitaker-package-installerappear in the binary targets forwhitaker-installer.package_lintsandpackage_installer_binare not used.package_binary_target_names(package_name)to derive explicit Cargo binary target names for a workspace package viacargo metadata.Why
How to test
cargo metadatareturns bin targets for thewhitaker-installerpackage that include:whitaker-installer,whitaker-package-lints,whitaker-package-installer.tests/workflows/test_rolling_release_workflow.pypass with the new contract.Additional notes
📎 Task: https://www.devboxer.com/task/ade4b7ad-b1d1-40cb-9141-2e89550f891e