Broaden no_expect lint for path-loaded test modules (tokio, rstest, cfg)#191
Open
Broaden no_expect lint for path-loaded test modules (tokio, rstest, cfg)#191
Conversation
Extend test module name detection to include common conventions like `service_tests` and `api_test`. This allows `.expect(...)` calls inside `#[tokio::test]` functions within `#[path]`-loaded modules that have non-standard names, supporting the Rust `--test` harness fallback. Add regression examples and UI tests to validate these cases, covering issue #132. This improves the lint's accuracy in identifying test contexts beyond standard `test`/`tests` modules. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
OverviewThis PR broadens the no_expect_outside_tests lint to correctly recognise test contexts inside modules loaded via #[path] that use non-standard names (e.g., service_tests, api_test) when compiled under the --test harness. It addresses issue Core logic changescrates/no_expect_outside_tests/src/driver/mod.rs
Test and verificationcrates/no_expect_outside_tests/src/driver/tests.rs
crates/no_expect_outside_tests/src/lib_ui_tests.rs
UI/regression fixtures and examples
Test-like attribute recognitioncommon/src/attributes/mod.rs and common/src/attributes/tests.rs
Docs
Why this mattersPreviously, path-loaded modules with non-standard names could bypass module-name checks so test contexts (e.g., functions annotated with #[tokio::test] or rstest-generated companions) were not always recognised, causing spurious diagnostics for .expect(...). This change broadens detection to cover common non-standard naming patterns and rstest companion modules while retaining prior recognition of explicit test attributes and cfg(test) modules. How to testRun cargo test -p no_expect_outside_tests and the repository UI/regression tests; the added unit, UI and example fixtures exercise the new detection logic. RelatedFixes WalkthroughExtend test-context detection for the Changes
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 6 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (6 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 GuideBroadens test-context detection in the no_expect_outside_tests lint (including rstest and #[path]-loaded modules with non-standard names) and adds regression coverage via unit, UI, and example-based tests to ensure Option::expect is only allowed in legitimate test code. Sequence diagram for linting path-loaded tokio test modulessequenceDiagram
actor Developer
participant RustcTestHarness
participant NoExpectLintDriver
participant Module_service_tests
Developer->>RustcTestHarness: cargo test (with --test)
RustcTestHarness->>NoExpectLintDriver: Run no_expect_outside_tests lint
NoExpectLintDriver->>Module_service_tests: Visit path-loaded module service_tests
NoExpectLintDriver->>NoExpectLintDriver: is_test_named_module("service_tests")
NoExpectLintDriver->>NoExpectLintDriver: has_test_module_name("service_tests")
NoExpectLintDriver-->>NoExpectLintDriver: true (matches suffix _tests)
NoExpectLintDriver->>Module_service_tests: Treat as test context
Module_service_tests-->>NoExpectLintDriver: #[tokio::test] fn using Option::expect
NoExpectLintDriver-->>RustcTestHarness: Do not emit no_expect lint warning
RustcTestHarness-->>Developer: Tests compile and run successfully
Updated class diagram for no_expect_outside_tests driver and attributesclassDiagram
class Attributes {
<<module>>
+TEST_LIKE_PATHS : &[[&str]]
}
class NoExpectDriver {
<<module>>
+is_test_named_module(name: &str) bool
+has_test_module_name(name: &str) bool
+is_test_context(attrs: Attributes, cfg_flags: CfgFlags, in_test_harness: bool) bool
}
class CfgFlags {
<<struct>>
}
NoExpectDriver ..> Attributes : uses
NoExpectDriver ..> CfgFlags : reads
Flow diagram for has_test_module_name test-module detectionflowchart TD
A[Input module name] --> B{name == test or name == tests}
B -->|yes| G[Return true]
B -->|no| C{name starts with test_ or tests_}
C -->|yes| G
C -->|no| D{name ends with _test or _tests}
D -->|yes| G
D -->|no| H[Return false]
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.
…st context - Add recognition of rstest's companion test modules generated by #[case] in the test harness. - Include `rstest_parametrize` attribute for backwards compatibility. - Add a regression example using the actual rstest crate, demonstrating expect usage in tests. - Enhance documentation comment to explain the integration with rstest and test detection. - Enhance unit tests to accept rstest_parametrize attributes. - Add a UI test to validate end-to-end detection of rstest with #[case] attributes in test context. Closes: #189 Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rd test modules Replaced the `#[path]`-loaded external test module `service_tests` with an inline module in the `pass_expect_in_path_module_harness.rs` example. This change preserves testing of the `expect` usage inside non-standard test modules (e.g., named `service_tests`) without `#[cfg(test)]` in HIR. The refactoring is necessary because the `Test::example()` utility copies only a single `.rs` file to the temp directory and does not preserve subdirectories, so the `#[path]` approach is not compatible. This maintains the regression coverage for issue #132 related to harness descriptor fallback recognizing test functions in such modules. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
has_test_module_namebehavior and its tests seem inconsistent: the implementation only checkstest_/tests_prefixes and_test/_testssuffixes, buthas_test_module_name_matches_test_conventionsexpects "test_like_utils" to betrue—either adjust the predicate to cover this broader pattern or change the test/example to match the documented naming rules.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `has_test_module_name` behavior and its tests seem inconsistent: the implementation only checks `test_`/`tests_` prefixes and `_test`/`_tests` suffixes, but `has_test_module_name_matches_test_conventions` expects "test_like_utils" to be `true`—either adjust the predicate to cover this broader pattern or change the test/example to match the documented naming rules.
## Individual Comments
### Comment 1
<location path="crates/no_expect_outside_tests/src/driver/tests.rs" line_range="139-148" />
<code_context>
+
+use rstest::rstest;
+
+#[rstest]
+#[case(1)]
+#[case(42)]
</code_context>
<issue_to_address>
**suggestion (testing):** Add more edge-case names around `"test"`/`"tests"` substrings without underscores to lock in the intended `has_test_module_name` behaviour
The new `has_test_module_name_matches_test_conventions` cases cover exact, prefix, and suffix patterns well. To better pin down the boundary and prevent future regressions, consider adding a few negative cases like:
- `"mytest"` / `"mytests"` (no underscore) → `false`
- `"testx"` / `"testsx"` → `false`
- `"service_tests_helper"` (test-like suffix with extra tail) → `false` if that’s intended
This makes it clear that arbitrary `"test"`/`"tests"` substrings should not be treated as valid test module names.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+139
to
+148
| #[rstest] | ||
| #[case::exact_test("test", true)] | ||
| #[case::exact_tests("tests", true)] | ||
| #[case::prefix_test_helpers("test_helpers", true)] | ||
| #[case::prefix_tests_util("tests_util", true)] | ||
| #[case::suffix_service_tests("service_tests", true)] | ||
| #[case::suffix_api_test("api_test", true)] | ||
| #[case::suffix_integration_tests("integration_tests", true)] | ||
| #[case::suffix_unit_test("unit_test", true)] | ||
| #[case::plain_service("my_service", false)] |
There was a problem hiding this comment.
suggestion (testing): Add more edge-case names around "test"/"tests" substrings without underscores to lock in the intended has_test_module_name behaviour
The new has_test_module_name_matches_test_conventions cases cover exact, prefix, and suffix patterns well. To better pin down the boundary and prevent future regressions, consider adding a few negative cases like:
"mytest"/"mytests"(no underscore) →false"testx"/"testsx"→false"service_tests_helper"(test-like suffix with extra tail) →falseif that’s intended
This makes it clear that arbitrary "test"/"tests" substrings should not be treated as valid test module names.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5fae09ffa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+376
to
+378
| module.item_ids.iter().any(|id| { | ||
| let child = cx.tcx.hir_item(*id); | ||
| matches!(child.kind, hir::ItemKind::Const(..)) |
There was a problem hiding this comment.
The new has_companion_test_module path treats a function as test-only if a same-named sibling module contains any const item, which is too broad for --test builds. A non-test pair like fn foo() and mod foo { const LIMIT: usize = 3; } now gets foo inserted into harness_marked_test_functions, so .expect(...) inside foo is incorrectly allowed. This should match actual harness descriptors (similar to the stricter identity checks in is_matching_harness_test_descriptor) instead of accepting arbitrary constants.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_expect_outside_tests/examples/pass_expect_in_rstest_harness.rs`:
- Line 10: Update the module-level doc comment that mentions rstest_parametrize
by replacing the British spelling "recognises" with the repository standard
"recognizes" (i.e., change the verb in the line containing `rstest_parametrize`
in the module doc comment).
In `@crates/no_expect_outside_tests/src/driver/mod.rs`:
- Around line 355-381: Convert the leading explanatory line comments before
has_companion_test_module into a Rustdoc comment: replace the initial block of
`// rstest with #[case] expands...` and the following two `//` lines with a
`///` Rustdoc paragraph placed immediately above the fn
has_companion_test_module declaration so it matches the style used by
has_test_module_name and is picked up by cargo doc; leave the function signature
and body (references to cx, function_name, siblings, module, etc.) unchanged.
In `@crates/no_expect_outside_tests/src/lib_ui_tests.rs`:
- Around line 31-84: The three tests rstest_example_compiles_under_test_harness,
path_module_tokio_test_compiles_under_test_harness (and the third similar test)
duplicate the same harness logic; refactor by extracting the shared body into a
single parameterised test using #[rstest] and #[case] (e.g. a new function
test_example_compiles_under_test_harness with parameters case(example_name:
&str, case(panic_msg): &str)), call
whitaker::testing::ui::run_with_runner/run_test_runner/Test::example inside that
function using the case values, and replace the per-example panic strings with
formatted text built from the case parameters; remove the original duplicate
functions and ensure rustc_flags(["--test"]) and env_test_guard/with_vars_unset
calls remain unchanged inside the shared body.
In `@crates/no_expect_outside_tests/ui/pass_expect_in_cfg_test_named_module.rs`:
- Around line 13-17: The #[tokio::test] annotated function
tokio_expect_in_named_module is currently synchronous; update its signature to
be asynchronous by changing fn tokio_expect_in_named_module() to async fn
tokio_expect_in_named_module() so the tokio test runner can execute it properly
(leave the #[tokio::test] attribute and the body using option.expect unchanged).
🪄 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: 439ad4ed-cf2d-44c3-b0d5-b692de651f0e
📒 Files selected for processing (12)
common/src/attributes/mod.rscommon/src/attributes/tests.rscrates/no_expect_outside_tests/examples/pass_expect_in_path_module_harness.rscrates/no_expect_outside_tests/examples/pass_expect_in_rstest_harness.rscrates/no_expect_outside_tests/src/driver/mod.rscrates/no_expect_outside_tests/src/driver/tests.rscrates/no_expect_outside_tests/src/lib_ui_tests.rscrates/no_expect_outside_tests/ui/pass_expect_in_cfg_test_named_module.rscrates/no_expect_outside_tests/ui/pass_expect_in_cfg_test_named_module.stderrcrates/no_expect_outside_tests/ui/pass_expect_in_path_module_tokio_test.rscrates/no_expect_outside_tests/ui/pass_expect_in_path_module_tokio_test.stderrcrates/no_expect_outside_tests/ui/pass_expect_in_path_module_tokio_test/service_tests.module
crates/no_expect_outside_tests/examples/pass_expect_in_rstest_harness.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+355
to
+381
| // rstest with #[case] expands into a bare function plus a companion module | ||
| // of the same name containing harness const descriptors. Detect this pattern | ||
| // so the parent function is treated as test-only code. | ||
| fn has_companion_test_module<'tcx>( | ||
| cx: &LateContext<'tcx>, | ||
| function_name: Symbol, | ||
| siblings: &[&'tcx hir::Item<'tcx>], | ||
| ) -> bool { | ||
| siblings.iter().any(|sibling| { | ||
| let hir::ItemKind::Mod(_, module) = sibling.kind else { | ||
| return false; | ||
| }; | ||
| let Some(mod_ident) = sibling.kind.ident() else { | ||
| return false; | ||
| }; | ||
| if mod_ident.name != function_name { | ||
| return false; | ||
| } | ||
| // The module must contain at least one harness const descriptor to | ||
| // avoid false positives on non-test modules that happen to share a | ||
| // function name. | ||
| module.item_ids.iter().any(|id| { | ||
| let child = cx.tcx.hir_item(*id); | ||
| matches!(child.kind, hir::ItemKind::Const(..)) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Convert the leading comment to a Rustdoc comment.
The comment at lines 355-357 describes the function's purpose but uses // instead of ///. Use Rustdoc style for consistency with has_test_module_name and to enable cargo doc generation.
Proposed fix
-// rstest with #[case] expands into a bare function plus a companion module
-// of the same name containing harness const descriptors. Detect this pattern
-// so the parent function is treated as test-only code.
+/// Detect rstest companion module pattern.
+///
+/// rstest with `#[case]` expands into a bare function plus a companion module
+/// of the same name containing harness const descriptors. This pattern allows
+/// the parent function to be treated as test-only code.
fn has_companion_test_module<'tcx>(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // rstest with #[case] expands into a bare function plus a companion module | |
| // of the same name containing harness const descriptors. Detect this pattern | |
| // so the parent function is treated as test-only code. | |
| fn has_companion_test_module<'tcx>( | |
| cx: &LateContext<'tcx>, | |
| function_name: Symbol, | |
| siblings: &[&'tcx hir::Item<'tcx>], | |
| ) -> bool { | |
| siblings.iter().any(|sibling| { | |
| let hir::ItemKind::Mod(_, module) = sibling.kind else { | |
| return false; | |
| }; | |
| let Some(mod_ident) = sibling.kind.ident() else { | |
| return false; | |
| }; | |
| if mod_ident.name != function_name { | |
| return false; | |
| } | |
| // The module must contain at least one harness const descriptor to | |
| // avoid false positives on non-test modules that happen to share a | |
| // function name. | |
| module.item_ids.iter().any(|id| { | |
| let child = cx.tcx.hir_item(*id); | |
| matches!(child.kind, hir::ItemKind::Const(..)) | |
| }) | |
| }) | |
| } | |
| /// Detect rstest companion module pattern. | |
| /// | |
| /// rstest with `#[case]` expands into a bare function plus a companion module | |
| /// of the same name containing harness const descriptors. This pattern allows | |
| /// the parent function to be treated as test-only code. | |
| fn has_companion_test_module<'tcx>( | |
| cx: &LateContext<'tcx>, | |
| function_name: Symbol, | |
| siblings: &[&'tcx hir::Item<'tcx>], | |
| ) -> bool { | |
| siblings.iter().any(|sibling| { | |
| let hir::ItemKind::Mod(_, module) = sibling.kind else { | |
| return false; | |
| }; | |
| let Some(mod_ident) = sibling.kind.ident() else { | |
| return false; | |
| }; | |
| if mod_ident.name != function_name { | |
| return false; | |
| } | |
| // The module must contain at least one harness const descriptor to | |
| // avoid false positives on non-test modules that happen to share a | |
| // function name. | |
| module.item_ids.iter().any(|id| { | |
| let child = cx.tcx.hir_item(*id); | |
| matches!(child.kind, hir::ItemKind::Const(..)) | |
| }) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/no_expect_outside_tests/src/driver/mod.rs` around lines 355 - 381,
Convert the leading explanatory line comments before has_companion_test_module
into a Rustdoc comment: replace the initial block of `// rstest with #[case]
expands...` and the following two `//` lines with a `///` Rustdoc paragraph
placed immediately above the fn has_companion_test_module declaration so it
matches the style used by has_test_module_name and is picked up by cargo doc;
leave the function signature and body (references to cx, function_name,
siblings, module, etc.) unchanged.
crates/no_expect_outside_tests/ui/pass_expect_in_cfg_test_named_module.rs
Show resolved
Hide resolved
…ion; unify UI example test harnesses - Refined logic detecting rstest companion test modules to verify matching harness const descriptors. - Adjusted test case attributes for module name matching. - Replaced multiple UI test functions with a single parameterized rstest in lib_ui_tests. - Changed async test function signature in cfg test named module example. - Minor docs and spelling fixes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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 `@crates/no_expect_outside_tests/src/driver/mod.rs`:
- Around line 1-7: This file is too large; extract the test-related helpers and
harness collection logic into a new submodule to reduce mod.rs below 400 lines:
move functions has_test_module_name and has_companion_test_module plus the code
that gathers/filters test harnesses (the logic that inspects attributes,
cfg(test) guards, and harness descriptors / Crate::is_doctest handling) into a
new file driver/test_context.rs, expose only the minimal API (e.g., pub fn
has_test_module_name, pub fn has_companion_test_module, and a function to
collect/identify harness contexts) and update mod.rs to use that submodule.
- Around line 233-237: The doc comment starting with "Recognise common test
module naming conventions." should use en-GB-oxendict '-ize' spelling; change
"Recognise" to "Recognize" in that comment block (the doc comment that continues
with "Matches exact names (`test`, `tests`)..."). Update the comment text
accordingly so the header reads "Recognize common test module naming
conventions."
In `@crates/no_expect_outside_tests/src/driver/tests.rs`:
- Line 152: The test case labeled test_embedded_not_suffix is asserting the
wrong expectation: the input "test_like_utils" begins with "test_" so
has_test_module_name(...) will return true; update the case either by changing
the expected boolean from false to true for the case named
test_embedded_not_suffix or replace the input with a true negative (e.g. a
string that does not start with "test_") so the case name and expectation match;
locate the case attribute in crates/no_expect_outside_tests/src/driver/tests.rs
(the #[case::test_embedded_not_suffix("test_like_utils", false)] line) and make
the corresponding change.
🪄 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: 882583c6-d6bf-4dd1-89a0-ee63a099baff
📒 Files selected for processing (6)
crates/no_expect_outside_tests/examples/pass_expect_in_rstest_harness.rscrates/no_expect_outside_tests/src/driver/mod.rscrates/no_expect_outside_tests/src/driver/tests.rscrates/no_expect_outside_tests/src/lib_ui_tests.rscrates/no_expect_outside_tests/ui/pass_expect_in_cfg_test_named_module.rsdocs/adr-002-dylint-expect-attribute-macro.md
Comment on lines
1
to
+7
| //! Lint crate forbidding `.expect(..)` outside test and doctest contexts. | ||
| //! | ||
| //! The lint inspects method calls named `expect`, verifies that the receiver | ||
| //! is an `Option` or `Result`, and checks the surrounding traversal context for | ||
| //! test-like attributes or `cfg(test)` guards. Doctest harnesses are skipped via | ||
| //! `Crate::is_doctest`, ensuring documentation examples remain ergonomic. When | ||
| //! no test context is present, the lint emits a denial with a note describing | ||
| //! the enclosing function and the receiver type to guide remediation. Teams can | ||
| //! extend the recognised test attributes through `dylint.toml` when bespoke | ||
| //! macros are in play. | ||
| //! The lint inspects method calls named `expect`, verifies the receiver is an | ||
| //! `Option` or `Result`, and checks the surrounding context for test-like | ||
| //! attributes, `cfg(test)` guards, or harness descriptors. Doctest harnesses | ||
| //! are skipped via `Crate::is_doctest`. Teams can extend the recognised test | ||
| //! attributes through `dylint.toml` when bespoke macros are in play. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
File exceeds 400-line limit.
At 417 lines, this file breaches the repository guideline of 400 lines maximum. Extract related helper functions (e.g., has_test_module_name, has_companion_test_module, and the harness collection logic) into a dedicated submodule such as driver/test_context.rs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/no_expect_outside_tests/src/driver/mod.rs` around lines 1 - 7, This
file is too large; extract the test-related helpers and harness collection logic
into a new submodule to reduce mod.rs below 400 lines: move functions
has_test_module_name and has_companion_test_module plus the code that
gathers/filters test harnesses (the logic that inspects attributes, cfg(test)
guards, and harness descriptors / Crate::is_doctest handling) into a new file
driver/test_context.rs, expose only the minimal API (e.g., pub fn
has_test_module_name, pub fn has_companion_test_module, and a function to
collect/identify harness contexts) and update mod.rs to use that submodule.
Comment on lines
+233
to
+237
| /// Recognise common test module naming conventions. | ||
| /// | ||
| /// Matches exact names (`test`, `tests`) as well as modules whose name starts | ||
| /// with `test_` or `tests_`, or ends with `_test` or `_tests`. This covers | ||
| /// `#[path]`-loaded modules with non-standard names such as `service_tests` |
There was a problem hiding this comment.
Fix en-GB-oxendict spelling: use "recognize".
The coding guidelines mandate -ize spelling. Replace "Recognise" with "Recognize" on line 233.
Proposed fix
-/// Recognise common test module naming conventions.
+/// Recognize common test module naming conventions.As per coding guidelines: "Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Recognise common test module naming conventions. | |
| /// | |
| /// Matches exact names (`test`, `tests`) as well as modules whose name starts | |
| /// with `test_` or `tests_`, or ends with `_test` or `_tests`. This covers | |
| /// `#[path]`-loaded modules with non-standard names such as `service_tests` | |
| /// Recognize common test module naming conventions. | |
| /// | |
| /// Matches exact names (`test`, `tests`) as well as modules whose name starts | |
| /// with `test_` or `tests_`, or ends with `_test` or `_tests`. This covers | |
| /// `#[path]`-loaded modules with non-standard names such as `service_tests` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/no_expect_outside_tests/src/driver/mod.rs` around lines 233 - 237, The
doc comment starting with "Recognise common test module naming conventions."
should use en-GB-oxendict '-ize' spelling; change "Recognise" to "Recognize" in
that comment block (the doc comment that continues with "Matches exact names
(`test`, `tests`)..."). Update the comment text accordingly so the header reads
"Recognize common test module naming conventions."
| #[case::testing("testing", false)] | ||
| #[case::attest("attest", false)] | ||
| #[case::contest("contest", false)] | ||
| #[case::test_embedded_not_suffix("test_like_utils", false)] |
There was a problem hiding this comment.
Test case expectation is incorrect.
"test_like_utils" starts with "test_", so has_test_module_name returns true. The expected value should be true, not false, or the case name and input should be changed to a genuinely negative example.
Proposed fix (option A: correct the expectation)
-#[case::test_embedded_not_suffix("test_like_utils", false)]
+#[case::test_prefix_with_suffix("test_like_utils", true)]Proposed fix (option B: use a different negative example)
-#[case::test_embedded_not_suffix("test_like_utils", false)]
+#[case::test_embedded_not_affix("utils_testing_helpers", false)]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[case::test_embedded_not_suffix("test_like_utils", false)] | |
| #[case::test_prefix_with_suffix("test_like_utils", true)] |
Suggested change
| #[case::test_embedded_not_suffix("test_like_utils", false)] | |
| #[case::test_embedded_not_affix("utils_testing_helpers", false)] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/no_expect_outside_tests/src/driver/tests.rs` at line 152, The test
case labeled test_embedded_not_suffix is asserting the wrong expectation: the
input "test_like_utils" begins with "test_" so has_test_module_name(...) will
return true; update the case either by changing the expected boolean from false
to true for the case named test_embedded_not_suffix or replace the input with a
true negative (e.g. a string that does not start with "test_") so the case name
and expectation match; locate the case attribute in
crates/no_expect_outside_tests/src/driver/tests.rs (the
#[case::test_embedded_not_suffix("test_like_utils", false)] line) and make the
corresponding change.
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
#[path]-loaded modules with non-standard names (e.g.,service_tests) when compiling under the--testharness. This ensuresOption::expectlint rules are applied appropriately in nested path-modules containing#[tokio::test]functions.Changes
Core logic
has_test_module_name(name: &str) -> boolincrates/no_expect_outside_tests/src/driver/mod.rsto recognise test module naming conventions:test,teststest_,tests__test,_testsis_test_named_moduleto delegate tohas_test_module_nameinstead of hard-coded checks.Tests and fixtures
crates/no_expect_outside_tests/src/driver/tests.rswithhas_test_module_name_matches_test_conventionsusingrstestto cover a broad set of naming patterns, includingservice_testsand other non-standard forms.--test:crates/no_expect_outside_tests/examples/pass_expect_in_path_module_harness.rsand support modulepath_module_harness_support/service_tests.rsto simulate a path-loaded test module.crates/no_expect_outside_tests/ui/pass_expect_in_path_module_tokio_test.rsand accompanyingservice_tests.moduleto exercise#[tokio::test]inside a path-loaded module.crates/no_expect_outside_tests/ui/pass_expect_in_cfg_test_named_module.rsand related stderr fixture to verify#[cfg(test)]-guarded non-standard module names.path_module_tokio_test_compiles_under_test_harnessto verify that a path-loaded tokio test compiles under the test harness (UI test runner).Examples
crates/no_expect_outside_tests/examples/pass_expect_in_path_module_harness.rscrates/no_expect_outside_tests/examples/path_module_harness_support/service_tests.rsWhy this matters
service_tests) that arise from#[path]-loaded tests, ensuring the lint behavior is consistent across all test structures.How to test
#[tokio::test]are recognized as test contexts under--test).examples/to reproduce the path-loaded module scenario described in the regression tests.Related issues
no_expect_outside_testsdoes not recognise#[tokio::test]#132◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/6740313c-27f0-4e46-a021-2a6a9f9aca06
📝 Closes #132
Summary by Sourcery
Broaden detection of test-only code for the no_expect_outside_tests lint, especially for rstest-based tests and non-standard, path-loaded test modules, and add regression coverage to ensure correct behavior under the --test harness.
New Features:
Enhancements:
Tests: