Skip to content

Create rstest helper fixture lint crate (8.2.1)#231

Open
leynos wants to merge 16 commits into
mainfrom
8-2-1-create-the-rstest-helper-lint-crate
Open

Create rstest helper fixture lint crate (8.2.1)#231
leynos wants to merge 16 commits into
mainfrom
8-2-1-create-the-rstest-helper-lint-crate

Conversation

@leynos
Copy link
Copy Markdown
Owner

@leynos leynos commented May 18, 2026

Summary

This branch implements roadmap task (8.2.1) by adding the experimental rstest_helper_should_be_fixture Dylint crate and registering RSTEST_HELPER_SHOULD_BE_FIXTURE without yet implementing call-site diagnostics.

ExecPlan: docs/execplans/8-2-1-create-the-rstest-helper-lint-crate.md

What changed

  • Added crates/rstest_helper_should_be_fixture with configuration defaults and unit coverage for TOML loading, normalization, provider attributes, and detection options.
  • Wired the lint into installer experimental resolution and the suite feature experimental-rstest-helper-should-be-fixture.
  • Enforced --experimental for explicit experimental lint builds such as --lint rstest_helper_should_be_fixture.
  • Resolved still-valid inline review comments for stub module docs, users-guide coverage, design-document ordering, and dynamic experimental feature expectations.
  • Updated user, developer, design, roadmap, and ExecPlan documentation for the experimental bootstrap behaviour.

Validation

  • cargo check -p rstest_helper_should_be_fixture --all-targets --all-features: passed
  • cargo check -p whitaker_suite --all-targets --all-features: passed
  • cargo nextest run -p rstest_helper_should_be_fixture --all-targets --all-features: passed, 9 tests
  • cargo nextest run -p whitaker_suite -p whitaker-installer --all-targets --all-features: passed, 527 tests
  • cargo nextest run -p whitaker-installer --all-targets --all-features -E 'binary(behaviour_core)': passed, 12 tests
  • cargo nextest run -p whitaker-installer --all-targets --all-features --profile ci -E 'test(scenario_dry_run_rejects_experimental_lint_without_opt_in)': passed, 1 test
  • cargo nextest run -p rstest_helper_should_be_fixture -p whitaker-installer --all-targets --all-features builder::tests::features_for_crate_handles_suite_experimental_mode: passed, 1 test
  • make check-fmt: passed
  • make markdownlint: passed with 0 errors
  • make lint: passed
  • make test: passed, 1431 tests passed and 2 skipped under the default nextest profile

Review notes

CodeRabbit was run after the crate/bootstrap milestone and again before final validation. Reported concerns were fixed: threshold normalization now preserves repeated-call semantics by clamping to at least 2, provider defaults no longer allocate a whole default config for fallback, and the private Config boundary is documented. CodeRabbit also reviewed the explicit experimental lint opt-in fix and the inline-review follow-up; both completed with 0 findings.

Inline comments were verified with Wyvern and Scribe agents before changes were made. The historical ExecPlan 1418 count was left unchanged because it records the earlier plan milestone; later implementation validation in the same document records the authoritative 1428 and final 1431 counts.

References

Summary by Sourcery

Add an experimental rstest_helper_should_be_fixture lint crate, wire it into the suite and installer as a feature-gated experimental lint, and document its configuration and opt-in behaviour.

New Features:

  • Introduce the rstest_helper_should_be_fixture Dylint crate with configuration defaults and bootstrap driver for future rstest helper-to-fixture recommendations.

Bug Fixes:

  • Ensure explicit installer builds of experimental lints (via --lint) are rejected unless --experimental is provided, matching documented opt-in requirements.

Enhancements:

  • Wire the experimental rstest helper lint into the suite via a feature-gated dependency and lint registration.
  • Extend the installer resolution and builder logic to recognise experimental lint crates and derive matching suite features.
  • Clarify developer and user documentation around experimental lints, their configuration, and how the installer enables them via --experimental.

Documentation:

  • Document the experimental rstest_helper_should_be_fixture lint, its purpose, configuration options, and experimental status in the user and design guides.
  • Update the roadmap and execution plan docs to mark 8.2.1 complete and record implementation decisions for the new lint.

Tests:

  • Add unit tests for the new lint crate configuration loading and normalization, and behavioural tests for installer validation and CLI error handling around experimental lints.
  • Extend suite and installer registration tests to cover experimental lint inclusion under the --experimental flag.

Add the pre-implementation ExecPlan for roadmap item 8.2.1. The plan
captures the scope, experimental wiring decision, validation steps, review
requirements, and approval gate for creating the
`rstest_helper_should_be_fixture` lint crate.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

Review Change Stack

Warning

Rate limit exceeded

@lodyai[bot] has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 38 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7402cca7-d2d6-410b-8146-6425e7ac7930

📥 Commits

Reviewing files that changed from the base of the PR and between e60fec1 and 1083113.

📒 Files selected for processing (1)
  • crates/rstest_helper_should_be_fixture/src/driver.rs

Walkthrough

Introduce an experimental Dylint lint crate rstest_helper_should_be_fixture that loads configuration from dylint.toml, normalises detection options, and integrates into the suite and installer so it is enabled only when the --experimental flag or the suite feature is used.

Changes

Experimental rstest_helper_should_be_fixture lint crate bootstrap

Layer / File(s) Summary
Lint crate scaffold and driver bootstrap
crates/rstest_helper_should_be_fixture/Cargo.toml, crates/rstest_helper_should_be_fixture/src/lib.rs, crates/rstest_helper_should_be_fixture/src/driver.rs
New crate manifest configures library types (cdylib/rlib), feature gates (dylint-driver, constituent), and optional dependencies. Library module conditionally wires the driver module when the dylint-driver feature is enabled, or provides a stub when disabled. Driver module defines Config with normalisation methods for thresholds and provider attributes, registers RstestHelperShouldBeFixture as a Warn lint pass that loads configuration and initialises detection options during check_crate, and includes unit tests for configuration deserialisation, threshold clamping, attribute normalisation, and detection-option expansion.
Suite and installer integration wiring
suite/Cargo.toml, suite/src/driver.rs, suite/src/lints.rs, installer/src/resolution.rs, installer/src/builder.rs, installer/src/main.rs, installer/src/error.rs
Suite adds the experimental-rstest-helper-should-be-fixture feature and optional dependency, conditionally includes RstestHelperShouldBeFixture in the combined late-lint pass, and registers the lint in SUITE_LINTS and SUITE_LINT_DECLS under the feature gate. Installer populates EXPERIMENTAL_LINT_CRATES with the new crate name, adds is_experimental_crate(), and changes validate_crate_names() to accept CrateResolutionOptions so experimental crates are rejected unless options.experimental is true. Unit and builder tests updated to reflect the experimental feature behaviour.
Tests and behaviour scenarios
installer/tests/*, installer/tests/features/installer.feature
BDD and CLI tests extended: new steps, support helpers and scenarios validate experimental crate name handling and CLI dry‑run rejection when --experimental is not provided; behaviour tests now record validation error messages and the experimental flag in world state.
Documentation, execplan and roadmap
docs/users-guide.md, docs/developers-guide.md, docs/lints-for-rstest-fixtures-and-test-hygiene.md, docs/execplans/8-2-1-create-the-rstest-helper-lint-crate.md, docs/roadmap.md, README.md
Users and developers guides updated to document the experimental lint, feature‑gate semantics, example dylint.toml config block, and the installer opt‑in requirement. ExecPlan records scope, decisions and acceptance criteria for 8.2.1; roadmap marks 8.2.1 complete.

Sequence Diagram(s)

sequenceDiagram
    participant Installer as whitaker-installer
    participant Resolution as installer/src/resolution.rs
    participant SuiteCargo as suite/Cargo.toml
    participant SuiteDriver as suite/src/driver.rs
    participant RstestCrate as crates/rstest_helper_should_be_fixture

    Installer->>Resolution: validate_crate_names(names, options)
    Resolution->>Resolution: check EXPERIMENTAL_LINT_CRATES / is_experimental_crate
    Resolution-->>Installer: validation result (error if no --experimental)
    Installer->>SuiteCargo: enable feature experimental-rstest-helper-should-be-fixture
    SuiteCargo->>SuiteDriver: include optional rstest_helper_should_be_fixture dep
    SuiteDriver->>RstestCrate: activate dylint-driver feature
    RstestCrate->>RstestCrate: load Config and normalized() -> detection options
    SuiteDriver->>SuiteDriver: include RstestHelperShouldBeFixture in suite pass
Loading
Run tests affecting installer resolution, suite feature gates, and the new crate's unit tests.

Possibly related PRs

  • leynos/whitaker#145: Adjusts installer experimental-lint wiring and EXPERIMENTAL_LINT_CRATES semantics for a different experimental lint crate.

Poem

🎯 A fixture lint inhales the repo air,
Gated and quiet, its defaults set to keep,
Tests whisper checks and docs mark the step,
Installer guards the key — opt‑in required,
The roadmap ticks; the milestone sleeps.


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors)

Check name Status Explanation Resolution
Module-Level Documentation ❌ Error Test module in crates/rstest_helper_should_be_fixture/src/driver.rs lacks a docstring explaining its purpose and function. Add module-level docstring before the #[cfg(test)] mod tests definition, documenting the unit tests for driver configuration and detection options.
Unit Architecture ❌ Error check_crate hides load_configuration() and SharedConfig::load() failures. Query paths must not hide fallible I/O operations behind apparently pure APIs. Extract configuration and shared config loading to explicit boundaries. Make failures visible via Result returns or inject dependencies where fallibility is transparent.
✅ Passed checks (18 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarises the main change: creating the rstest helper fixture lint crate and implementing roadmap task 8.2.1 as required.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing the implementation of roadmap task 8.2.1, changes made, validation steps, and review resolutions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Testing (Overall) ✅ Passed Tests exercise changed behaviour. Config tests check normalisation and TOML validation. Installer tests validate experimental-lint errors. BDD tests verify CLI with/without --experimental flag.
User-Facing Documentation ✅ Passed Users-guide documents experimental lint with Purpose, Configuration, allowed/denied behaviours, and fixes. README signals the new optional lint. This opt-in feature requires no migration guide.
Developer Documentation ✅ Passed All requirements met: new APIs documented; developers-guide updated with experimental lint mechanism and feature checklist; ExecPlan comprehensive; roadmap item checked off.
Testing (Unit And Behavioural) ✅ Passed Unit tests cover config defaults, edge cases, error paths, invariants. Behavioural tests exercise functional boundaries through real CLI invocations testing experimental lint validation and workflows.
Testing (Property / Proof) ✅ Passed Property test added for idempotence of Config::normalized() using proptest over diverse input ranges. ExecPlan stated property tests unnecessary for 8.2.1; implementation exceeded requirements.
Testing (Compile-Time / Ui) ✅ Passed Diagnostic-silent bootstrap provides unit tests for configuration and behavioural tests for registration. UI tests appropriately deferred to later phases per ExecPlan constraints.
Domain Architecture ✅ Passed Domain logic properly segregated. Configuration rules are testable without rustc. Experimental status is installer/build concern only. Framework integration explicit and appropriate.
Observability ✅ Passed Operational scope is minimal and intentional: configuration loading (with debug-log on failure) and experimental-lint validation (with structured error reporting). Later phases defer analysis work.
Security And Privacy ✅ Passed No secrets, injection vulnerabilities, or data exposure. Experimental lint properly gated; Config validation enforces deny_unknown_fields; --experimental flag required and checked before any build.
Performance And Resource Use ✅ Passed No performance regressions found. Config loading follows established patterns from other lints. No unbounded collections, algorithmic regressions, or excessive allocations detected.
Concurrency And State ✅ Passed No concurrency concerns: lint crate has no shared state, async code, or locks. Each pass instance is isolated with state managed through &mut self only.
Architectural Complexity And Maintainability ✅ Passed All new abstractions justified and reduce complexity. Acyclic dependencies. Explicit maintenance story with no hidden hooks. Proper reuse of existing abstractions and battle-tested dependencies.
Rust Compiler Lint Integrity ✅ Passed Only one narrow #[expect(dead_code)] on feature-gated stub function. No #[allow(...)], no module-level suppressions, no lint-appeasement code detected.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 8-2-1-create-the-rstest-helper-lint-crate

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Sorry @leynos, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

codescene-delta-analysis[bot]

This comment was marked as outdated.

Update the ExecPlan progress section after opening the draft pull request.
This keeps the living plan aligned with the branch state without changing the
implementation scope.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Create the experimental `rstest_helper_should_be_fixture` Dylint crate
and wire it into the installer and suite behind the experimental feature.

Add configuration defaults and unit coverage for the bootstrap policy,
update registration behaviour coverage, and document the experimental
user-facing behaviour for roadmap item 8.2.1.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Update the ExecPlan status after pushing the implementation and refreshing
the draft PR for roadmap item 8.2.1.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Update the ExecPlan with the final post-push CodeRabbit confirmation
attempt, which remained blocked by a recoverable service rate limit.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos leynos marked this pull request as ready for review May 20, 2026 09:41
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Sorry @leynos, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai coderabbitai Bot added the Roadmap label May 20, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

Validate explicit `--lint` requests with the experimental option context so
experimental crates cannot bypass the documented `--experimental` gate.

Add core and CLI behaviour coverage for accepted opt-in requests and rejected
non-opt-in requests, and document the user-facing installer contract.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos leynos marked this pull request as draft May 20, 2026 11:40
coderabbitai[bot]

This comment was marked as resolved.

Fix the still-valid inline review comments for the experimental rstest helper
lint bootstrap. Add module documentation for the disabled-driver stub, derive
the suite experimental feature expectation from `EXPERIMENTAL_LINT_CRATES`, and
fill in the users-guide and design-document documentation gaps.

Record the verification outcome in the ExecPlan, including the stale historical
test-count comment that was intentionally left unchanged.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos leynos marked this pull request as ready for review May 21, 2026 20:37
@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 21, 2026

@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.

Large Method

installer/src/error.rs: Clone.clone

What lead to degradation?

Clone.clone increases from 85 to 88 lines of code, threshold = 70

Why does this problem occur?

Overly long functions make the code harder to read. The recommended maximum function length for the Rust language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method.

How to fix it?

We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern.

Update the ExecPlan validation summary so the plan milestone uses the same
1428 passed test count as the surrounding recorded validation notes.
@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented May 21, 2026

Reviewer's Guide

Introduces the experimental rstest_helper_should_be_fixture Dylint lint crate, wires it into the installer and suite as an experimental feature requiring explicit opt-in, and documents its configuration, purpose, and bootstrap-only (no diagnostics yet) behavior.

Sequence diagram for experimental rstest_helper_should_be_fixture lint opt-in and wiring

sequenceDiagram
    actor User
    participant InstallerCLI as installer_main
    participant Resolver as resolve_requested_crates
    participant Validator as validate_crate_names
    participant CrateResolver as resolve_crates
    participant Builder as features_for_crate
    participant Suite as whitaker_suite

    User->>InstallerCLI: run whitaker-installer --lint rstest_helper_should_be_fixture [--experimental]
    InstallerCLI->>Resolver: resolve_requested_crates(args)
    Resolver->>Validator: validate_crate_names(lint_crates, options)
    alt experimental flag missing
        Validator-->>Resolver: Err(ExperimentalLintRequiresFlag)
        Resolver-->>InstallerCLI: Err(InstallerError)
        InstallerCLI-->>User: fail: experimental lint requires --experimental
    else experimental flag present
        Validator-->>Resolver: Ok
        Resolver->>CrateResolver: resolve_crates(lint_crates, options)
        CrateResolver-->>Resolver: ["rstest_helper_should_be_fixture"]
        Resolver-->>InstallerCLI: Ok(crates)
        InstallerCLI->>Builder: features_for_crate("whitaker_suite") with experimental = true
        Builder-->>InstallerCLI: "dylint-driver,experimental-rstest-helper-should-be-fixture"
        InstallerCLI->>Suite: build whitaker_suite with feature experimental-rstest-helper-should-be-fixture
        Suite-->>User: suite cdylib including RSTEST_HELPER_SHOULD_BE_FIXTURE
    end
Loading

File-Level Changes

Change Details Files
Add experimental rstest_helper_should_be_fixture Dylint crate with configuration loading, normalization, and rstest detection options, but no diagnostics yet.
  • Create new crate manifest with dylint-driver and constituent features and workspace dependencies.
  • Implement driver that declares RSTEST_HELPER_SHOULD_BE_FIXTURE, defines TOML-backed Config with defaults, normalizes thresholds and provider attributes, and builds RstestDetectionOptions and localization.
  • Provide non-driver stub module for builds without dylint-driver feature.
  • Add unit tests for config defaults, deserialization, normalization, provider attribute handling, and detection options wiring.
crates/rstest_helper_should_be_fixture/Cargo.toml
crates/rstest_helper_should_be_fixture/src/lib.rs
crates/rstest_helper_should_be_fixture/src/driver.rs
Wire the new lint into the suite as a feature-gated experimental lint.
  • Add optional rstest_helper_should_be_fixture dependency and experimental-rstest-helper-should-be-fixture feature in suite Cargo.toml, depending on the lint crate and dylint-driver.
  • Update suite combined lint pass macro to include RstestHelperShouldBeFixture behind the experimental feature.
  • Update suite lint descriptors and lint declaration lists to include the new lint when the feature is enabled, and adjust suite lint name tests accordingly.
suite/Cargo.toml
suite/src/driver.rs
suite/src/lints.rs
Update installer resolution and builder to treat rstest_helper_should_be_fixture as an experimental lint requiring --experimental, including CLI/BDD coverage.
  • Populate EXPERIMENTAL_LINT_CRATES with rstest_helper_should_be_fixture and add is_experimental_crate helper.
  • Change validate_crate_names to accept CrateResolutionOptions, reject experimental crates without --experimental, and extend its tests for experimental cases and new error variant.
  • Add ExperimentalLintRequiresFlag installer error type and make it clonable.
  • Update main CLI to pass resolution options into validate_crate_names.
  • Extend builder feature derivation tests so suite features in experimental mode are built from EXPERIMENTAL_LINT_CRATES.
  • Add BDD worlds, scenarios, and CLI helpers asserting that dry-run with an experimental lint fails without opt-in and emits a specific error message.
installer/src/resolution.rs
installer/src/error.rs
installer/src/builder.rs
installer/src/main.rs
installer/tests/behaviour_core.rs
installer/tests/behaviour_cli.rs
installer/tests/behaviour_cli/support.rs
installer/tests/behaviour_cli/scenarios.rs
installer/tests/features/installer.feature
Document the new experimental lint, its configuration, and the experimental opt-in behavior across user, developer, design, roadmap, and ExecPlan docs.
  • Update users guide to explain experimental lints, how --experimental interacts with --individual-lints and explicit --lint, and to add configuration and behaviour sections for rstest_helper_should_be_fixture.
  • Update developers guide to describe experimental lint handling, suite feature naming, and steps for adding new experimental lints, including wiring steps for optional dependencies and gated descriptors.
  • Add implementation decisions section for 8.2.1 to the rstest fixture/test hygiene design doc, documenting that this milestone is bootstrap-only and that the suite feature is derived from EXPERIMENTAL_LINT_CRATES.
  • Mark roadmap item 8.2.1 as completed and add a detailed ExecPlan document describing constraints, decisions, progress, and validation for the new lint crate.
  • Adjust references to experimental lints and installer behavior to reflect the existence of this first experimental lint.
docs/users-guide.md
docs/developers-guide.md
docs/lints-for-rstest-fixtures-and-test-hygiene.md
docs/roadmap.md
docs/execplans/8-2-1-create-the-rstest-helper-lint-crate.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

sourcery-ai[bot]

This comment was marked as resolved.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@coderabbitai

This comment was marked as resolved.

Add CLI behaviour coverage for explicitly requesting the experimental rstest
helper lint with `--experimental` enabled. The suite macro redefinition comment
was already addressed by complementary cfg gates, so record that verification in
the ExecPlan without changing the suite driver.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Extract toolchain-specific clone handling and shared I/O error cloning
from `InstallerError::clone` so the implementation satisfies the local
method length threshold without changing clone behaviour.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Add focused tests around `rstest_helper_should_be_fixture` configuration
loading, pass initialisation, and normalisation idempotence. Clarify the
README lint count and experimental opt-in signpost so the public summary
matches the current lint set.
coderabbitai[bot]

This comment was marked as resolved.

codescene-delta-analysis[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as resolved.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 22, 2026

@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.

Please address the comments from this code review:

## Overall Comments
- In `suite/src/driver.rs` the `define_suite_pass!` macro is defined twice (one generic and one `#[cfg(feature = "experimental-rstest-helper-should-be-fixture")]`), which risks macro redefinition and makes the cfg logic harder to follow; consider keeping a single macro and applying `cfg` only to the lint list entries or to the call site instead.

## Individual Comments

### Comment 1
<location path="suite/src/driver.rs" line_range="17-24" />
<code_context>
 use no_expect_outside_tests::NoExpectOutsideTests;
 use no_std_fs_operations::NoStdFsOperations;
 use no_unwrap_or_else_panic::NoUnwrapOrElsePanic;
+#[cfg(feature = "experimental-rstest-helper-should-be-fixture")]
+use rstest_helper_should_be_fixture::RstestHelperShouldBeFixture;
 use test_must_not_have_example::TestMustNotHaveExample;

 dylint_library!();

+#[cfg(not(feature = "experimental-rstest-helper-should-be-fixture"))]
 macro_rules! define_suite_pass {
     () => {
         rustc_lint::late_lint_methods!(
</code_context>
<issue_to_address>
**issue (bug_risk):** Redefining the `define_suite_pass` macro under `cfg` is likely to cause a macro redefinition error.

There is an unconditional `macro_rules! define_suite_pass { ... }` and a second `#[cfg(feature = "experimental-rstest-helper-should-be-fixture")]`-guarded one with the same name. Because `cfg` does not remove the first definition, enabling the feature will produce a macro redefinition error. Instead, gate a single macro name with complementary `cfg`s, e.g.

```rust
#[cfg(feature = "experimental-rstest-helper-should-be-fixture")] 
macro_rules! define_suite_pass { /* experimental version */ }

#[cfg(not(feature = "experimental-rstest-helper-should-be-fixture"))]
macro_rules! define_suite_pass { /* non-experimental version */ }

define_suite_pass!();
```
</issue_to_address>

### Comment 2
<location path="installer/tests/behaviour_cli.rs" line_range="34-41" />
<code_context>
     configure_dry_run_unknown_lint(cli_world);
 }

+#[given("the installer is invoked with dry-run and an experimental lint")]
+fn given_dry_run_experimental_lint(cli_world: &CliWorld) {
+    configure_dry_run_experimental_lint(cli_world);
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a CLI scenario that covers an experimental lint with `--experimental` enabled

You’ve covered the failure mode where an experimental lint is passed without `--experimental`. To also verify the successful path, please add a complementary scenario in `installer.feature` (and wire it here) where `rstest_helper_should_be_fixture` is requested with `--experimental` set, asserting that dry-run output is produced and no experimental opt-in error appears.

Suggested implementation:

```rust
    assert_experimental_lint_opt_in_message_is_shown, assert_installation_succeeds_or_is_skipped,
    assert_suite_library_is_staged, assert_unknown_lint_message_is_shown,
    configure_dry_run_experimental_lint, configure_dry_run_experimental_lint_with_experimental_opt_in,
    configure_dry_run_unknown_lint, configure_dry_run_with_target_dir, configure_suite_install,
    is_toolchain_installed, pinned_toolchain_channel, run_installer_cli, workspace_root,
};

```

```rust
#[given("the installer is invoked with dry-run and an experimental lint")]
fn given_dry_run_experimental_lint(cli_world: &CliWorld) {
    configure_dry_run_experimental_lint(cli_world);
}

#[given("the installer is invoked with dry-run, an experimental lint, and experimental lints enabled")]
fn given_dry_run_experimental_lint_with_opt_in(cli_world: &CliWorld) {
    configure_dry_run_experimental_lint_with_experimental_opt_in(cli_world);
}

```

To fully implement the requested scenario, you should also:
1. Add `configure_dry_run_experimental_lint_with_experimental_opt_in` to the appropriate helper module (likely `installer/tests/cli_world.rs` or similar), configuring the CLI invocation to:
   - include the experimental lint `rstest_helper_should_be_fixture`,
   - enable `--dry-run`,
   - and pass `--experimental`.
2. Add a new scenario to `installer/tests/features/installer.feature` that uses the new Given step text:
   - `Given the installer is invoked with dry-run, an experimental lint, and experimental lints enabled`
   - Then steps should assert that:
     a) the dry-run output for the installation is produced (reuse existing “dry-run” assertion steps if available), and  
     b) **no** experimental lint opt-in error is shown (ensure this scenario does **not** use the `an experimental lint opt-in message is shown` step).
3. If you don’t already have a step to assert that no experimental lint opt-in message appears, consider either:
   - asserting positively on the expected dry-run content and absence of the specific error string in a new helper, or
   - extending an existing dry-run assertion helper to check that the experimental opt-in error is not present when `--experimental` is passed.
</issue_to_address>

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== suite/src/driver.rs ==="
cat suite/src/driver.rs

echo ""
echo "=== installer/tests/behaviour_cli.rs ==="
cat installer/tests/behaviour_cli.rs

echo ""
echo "=== installer/tests/features/installer.feature ==="
cat installer/tests/features/installer.feature

echo ""
echo "=== installer/tests/behaviour_cli/support.rs ==="
cat installer/tests/behaviour_cli/support.rs

Length of output: 27599


@leynos: Both issues are resolved.

Comment 1 — suite/src/driver.rs macro redefinition
Both definitions of define_suite_pass! now carry complementary cfg guards (#[cfg(not(feature = "..."))] and #[cfg(feature = "...")]), and both call sites are guarded identically. The macro redefinition risk is gone. ✅

Comment 2 — Successful-path experimental lint CLI scenario
All four required pieces are in place:

  • configure_dry_run_experimental_lint_with_opt_in in support.rs passes --dry-run, --experimental, --toolchain, --target-dir, and --lint rstest_helper_should_be_fixture. ✅
  • assert_experimental_lint_dry_run_output_is_shown asserts the dry-run banner, the crate name in output, and the absence of the opt-in error string. ✅
  • given_dry_run_experimental_lint_with_opt_in is wired in behaviour_cli.rs. ✅
  • installer.feature contains the "Dry-run accepts experimental lint with opt-in" scenario. ✅

Keep a single `define_suite_pass!` macro and pass the experimental rstest
helper lint entry only from the feature-gated call site. This avoids duplicate
macro definitions while preserving the existing feature-gated suite contents.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Move the rstest helper proptest dependency to the workspace table, tidy the
README project status wording, and fix the rstest helper documentation order
and grammar noted in review.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 22, 2026

@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.

Please address the comments from this code review:

## Overall Comments
- In `suite/src/driver.rs` the `define_suite_pass!` macro is defined twice (one generic and one `#[cfg(feature = "experimental-rstest-helper-should-be-fixture")]`), which risks macro redefinition and makes the cfg logic harder to follow; consider keeping a single macro and applying `cfg` only to the lint list entries or to the call site instead.

## Individual Comments

### Comment 1
<location path="suite/src/driver.rs" line_range="17-24" />
<code_context>
 use no_expect_outside_tests::NoExpectOutsideTests;
 use no_std_fs_operations::NoStdFsOperations;
 use no_unwrap_or_else_panic::NoUnwrapOrElsePanic;
+#[cfg(feature = "experimental-rstest-helper-should-be-fixture")]
+use rstest_helper_should_be_fixture::RstestHelperShouldBeFixture;
 use test_must_not_have_example::TestMustNotHaveExample;

 dylint_library!();

+#[cfg(not(feature = "experimental-rstest-helper-should-be-fixture"))]
 macro_rules! define_suite_pass {
     () => {
         rustc_lint::late_lint_methods!(
</code_context>
<issue_to_address>
**issue (bug_risk):** Redefining the `define_suite_pass` macro under `cfg` is likely to cause a macro redefinition error.

There is an unconditional `macro_rules! define_suite_pass { ... }` and a second `#[cfg(feature = "experimental-rstest-helper-should-be-fixture")]`-guarded one with the same name. Because `cfg` does not remove the first definition, enabling the feature will produce a macro redefinition error. Instead, gate a single macro name with complementary `cfg`s, e.g.

```rust
#[cfg(feature = "experimental-rstest-helper-should-be-fixture")] 
macro_rules! define_suite_pass { /* experimental version */ }

#[cfg(not(feature = "experimental-rstest-helper-should-be-fixture"))]
macro_rules! define_suite_pass { /* non-experimental version */ }

define_suite_pass!();
```
</issue_to_address>

### Comment 2
<location path="installer/tests/behaviour_cli.rs" line_range="34-41" />
<code_context>
     configure_dry_run_unknown_lint(cli_world);
 }

+#[given("the installer is invoked with dry-run and an experimental lint")]
+fn given_dry_run_experimental_lint(cli_world: &CliWorld) {
+    configure_dry_run_experimental_lint(cli_world);
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a CLI scenario that covers an experimental lint with `--experimental` enabled

You’ve covered the failure mode where an experimental lint is passed without `--experimental`. To also verify the successful path, please add a complementary scenario in `installer.feature` (and wire it here) where `rstest_helper_should_be_fixture` is requested with `--experimental` set, asserting that dry-run output is produced and no experimental opt-in error appears.

Suggested implementation:

```rust
    assert_experimental_lint_opt_in_message_is_shown, assert_installation_succeeds_or_is_skipped,
    assert_suite_library_is_staged, assert_unknown_lint_message_is_shown,
    configure_dry_run_experimental_lint, configure_dry_run_experimental_lint_with_experimental_opt_in,
    configure_dry_run_unknown_lint, configure_dry_run_with_target_dir, configure_suite_install,
    is_toolchain_installed, pinned_toolchain_channel, run_installer_cli, workspace_root,
};

```

```rust
#[given("the installer is invoked with dry-run and an experimental lint")]
fn given_dry_run_experimental_lint(cli_world: &CliWorld) {
    configure_dry_run_experimental_lint(cli_world);
}

#[given("the installer is invoked with dry-run, an experimental lint, and experimental lints enabled")]
fn given_dry_run_experimental_lint_with_opt_in(cli_world: &CliWorld) {
    configure_dry_run_experimental_lint_with_experimental_opt_in(cli_world);
}

```

To fully implement the requested scenario, you should also:
1. Add `configure_dry_run_experimental_lint_with_experimental_opt_in` to the appropriate helper module (likely `installer/tests/cli_world.rs` or similar), configuring the CLI invocation to:
   - include the experimental lint `rstest_helper_should_be_fixture`,
   - enable `--dry-run`,
   - and pass `--experimental`.
2. Add a new scenario to `installer/tests/features/installer.feature` that uses the new Given step text:
   - `Given the installer is invoked with dry-run, an experimental lint, and experimental lints enabled`
   - Then steps should assert that:
     a) the dry-run output for the installation is produced (reuse existing “dry-run” assertion steps if available), and  
     b) **no** experimental lint opt-in error is shown (ensure this scenario does **not** use the `an experimental lint opt-in message is shown` step).
3. If you don’t already have a step to assert that no experimental lint opt-in message appears, consider either:
   - asserting positively on the expected dry-run content and absence of the specific error string in a new helper, or
   - extending an existing dry-run assertion helper to check that the experimental opt-in error is not present when `--experimental` is passed.
</issue_to_address>

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== suite/src/driver.rs ==="
cat suite/src/driver.rs

echo ""
echo "=== installer/tests/behaviour_cli.rs ==="
cat installer/tests/behaviour_cli.rs

echo ""
echo "=== installer/tests/features/installer.feature ==="
cat installer/tests/features/installer.feature

echo ""
echo "=== installer/tests/behaviour_cli/support.rs ==="
cat installer/tests/behaviour_cli/support.rs

Length of output: 26466


@leynos: Both issues are resolved.

Comment 1 — suite/src/driver.rs macro redefinition
The implementation is cleaner than the suggested complementary-definition approach: a single macro_rules! define_suite_pass! accepting variadic $($experimental_pass:tt)* tokens is defined once, and the two cfg-gated call sites (define_suite_pass!() vs define_suite_pass!(RstestHelperShouldBeFixture: ...)) select the correct expansion. There is no duplicate definition and no redefinition risk. ✅

Comment 2 — Successful-path experimental lint CLI scenario
All four required pieces are present:

  • configure_dry_run_experimental_lint_with_opt_in in support.rs passes --dry-run, --experimental, --toolchain, --target-dir, and --lint rstest_helper_should_be_fixture. ✅
  • assert_experimental_lint_dry_run_output_is_shown asserts the dry-run banner, the crate name in output, and the absence of the opt-in error string. ✅
  • given_dry_run_experimental_lint_with_opt_in is wired in behaviour_cli.rs. ✅
  • installer.feature contains the "Dry-run accepts experimental lint with opt-in" scenario. ✅

Return a `Result` from the rstest helper lint configuration boundary so
parse failures are visible before the driver falls back to defaults. Document
the driver unit-test module now that it covers configuration loading and
`rstest` detection option construction.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

Route `SharedConfig::load` through a private helper so the Dylint driver has an
explicit boundary for shared configuration loading. Document that the current
shared loader is treated as infallible at this call site until a follow-up
makes I/O failures visible.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 22, 2026

@coderabbitai Have the following now been resolved?

crates/rstest_helper_should_be_fixture/Cargo.toml (1)

38-43: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Centralise proptest as a workspace dependency for consistency.
All other dev-dependencies use { workspace = true } whereas proptest specifies a direct version. If proptest is used elsewhere in the workspace, add it to the root Cargo.toml workspace dependencies and reference it here as proptest = { workspace = true }.

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @crates/rstest_helper_should_be_fixture/Cargo.toml around lines 38 - 43, The
proptest entry in this Cargo.toml is declared with a version string instead of
referencing the workspace like the other dev-dependencies; add proptest to the
root Cargo.toml workspace dev-dependencies (or [workspace.dependencies]) and
then change the local entry here from proptest = "1" to proptest = { workspace =
true } under [dev-dependencies] so it matches the other workspace-referenced
crates.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== Root Cargo.toml (workspace.dependencies section) ==="
grep -n "proptest" Cargo.toml

echo ""
echo "=== crates/rstest_helper_should_be_fixture/Cargo.toml ==="
cat crates/rstest_helper_should_be_fixture/Cargo.toml

Length of output: 1327


@leynos: Yes, this is resolved. ✅

  • proptest = "1" is declared in the root [workspace.dependencies] (line 40 of Cargo.toml).
  • crates/rstest_helper_should_be_fixture/Cargo.toml references it as proptest = { workspace = true } under [dev-dependencies], consistent with all other dev-dependencies in that file.

@coderabbitai

This comment was marked as resolved.

Replace the placeholder follow-up reference in the shared configuration
boundary notes with the tracked GitHub issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant