fix(lint): consider more constructs as valid test assertions#9178
fix(lint): consider more constructs as valid test assertions#9178dyc3 merged 1 commit intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: c70d97f The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedFailed to post review comments Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR broadens Biome's JS test-assertion recognition by treating Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_expect.rs (1)
44-44: Trailing whitespace in doc comment.Line 44 appears to have trailing whitespace after
///. Runningjust fshould clean this up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/use_expect.rs` at line 44, Remove the trailing whitespace in the doc comment that currently contains "/// " in crates/biome_js_analyze/src/lint/nursery/use_expect.rs (the standalone doc-comment line shown in the diff); edit that line to be exactly "///" with no trailing spaces and re-run the formatter (just f) to ensure the file is clean..changeset/busy-numbers-reply.md (1)
5-5: Minor grammatical tweak in the changeset title.The title reads "Fixed consider more constructs..." which is a bit awkward. Consider rephrasing to "Fix: consider more constructs as valid test assertions" or simply "Consider more constructs as valid test assertions".
Suggested fix
-Fixed consider more constructs as valid test assertions +Fix: consider more constructs as valid test assertions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/busy-numbers-reply.md at line 5, Update the changeset title string "Fixed consider more constructs as valid test assertions" to a clearer phrasing—either "Fix: consider more constructs as valid test assertions" (preferred conventional changeset prefix) or "Consider more constructs as valid test assertions" so the title reads naturally; edit the single-line title in the changeset content to match one of these suggestions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/busy-numbers-reply.md:
- Around line 14-16: The test uses incorrect TypeScript generic syntax by
calling expectTypeOf<myStr>() where myStr is a const value; change the assertion
to use a value or its type instead — either call
expectTypeOf(myStr).toBeString() or expectTypeOf<typeof myStr>().toBeString(),
updating the test that references myStr and expectTypeOf accordingly.
In `@crates/biome_js_analyze/src/lint/nursery/use_expect.rs`:
- Around line 48-50: The example uses a runtime function name directly; update
the example to query the type of the runtime value by wrapping the function name
with typeof in the expectTypeOf call — replace
expectTypeOf<myFunc>().toBeCallableWith("foo") with expectTypeOf<typeof
myFunc>().toBeCallableWith("foo") so the test explicitly references the runtime
function type (symbols to change: expectTypeOf, myFunc, toBeCallableWith).
---
Nitpick comments:
In @.changeset/busy-numbers-reply.md:
- Line 5: Update the changeset title string "Fixed consider more constructs as
valid test assertions" to a clearer phrasing—either "Fix: consider more
constructs as valid test assertions" (preferred conventional changeset prefix)
or "Consider more constructs as valid test assertions" so the title reads
naturally; edit the single-line title in the changeset content to match one of
these suggestions.
In `@crates/biome_js_analyze/src/lint/nursery/use_expect.rs`:
- Line 44: Remove the trailing whitespace in the doc comment that currently
contains "/// " in crates/biome_js_analyze/src/lint/nursery/use_expect.rs (the
standalone doc-comment line shown in the diff); edit that line to be exactly
"///" with no trailing spaces and re-run the formatter (just f) to ensure the
file is clean.
ematipico
left a comment
There was a problem hiding this comment.
Thank you. Code looks good, just need to tweak the changeset to follow our requirements
Merging this PR will not alter performance
Comparing Footnotes
|
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 @.changeset/busy-numbers-reply.md:
- Line 8: The markdown line containing "assert", "expectTypeOf" and "assertType"
has a trailing space causing the linter to fail; open the file, locate the line
with those tokens and remove the trailing whitespace at the end of that line
(ensure no extra spaces after the closing parenthesis), save and re-run the
pipeline to confirm the Markdown linter passes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_expect.rs (1)
11-68: Consider aligning the diagnostic wording with the new “allowed variants” docs.Docs now mention
assert/expectTypeOf/assertType; the diagnostic still says onlyexpect(). A small tweak could prevent confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/use_expect.rs` around lines 11 - 68, The diagnostic message in the declare_lint_rule! for the use_expect rule (the rule defined in this file—referenced by the declare_lint_rule! block / rule name use_expect) still mentions only `expect()`; update the diagnostic text emitted by this rule to mention the allowed variants `assert`, `expectTypeOf`, and `assertType` (or a short phrase like "expect(), assert(), expectTypeOf(), assertType()") so it matches the documentation and avoid confusion; locate the diagnostic message string inside the declare_lint_rule! (the message used when reporting tests without assertions) and edit it to include those variants and/or a brief pointer to the docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_expect.rs`:
- Around line 11-68: The diagnostic message in the declare_lint_rule! for the
use_expect rule (the rule defined in this file—referenced by the
declare_lint_rule! block / rule name use_expect) still mentions only `expect()`;
update the diagnostic text emitted by this rule to mention the allowed variants
`assert`, `expectTypeOf`, and `assertType` (or a short phrase like "expect(),
assert(), expectTypeOf(), assertType()") so it matches the documentation and
avoid confusion; locate the diagnostic message string inside the
declare_lint_rule! (the message used when reporting tests without assertions)
and edit it to include those variants and/or a brief pointer to the docs.
|
Why is this against |
|
Oops |
|
This needs to be rebased onto main btw |
1b0e1a9 to
c70d97f
Compare
|
@ematipico @dyc3 |
Summary
Fixes #9172
Fixes #9168
Test Plan
Added new tests for allowed constructs
Docs
More doctests were added as well.