Skip to content

chore(linter): Add more test cases to the no-with rule.#16791

Merged
graphite-app[bot] merged 1 commit intomainfrom
add-more-tests-for-no-with
Dec 13, 2025
Merged

chore(linter): Add more test cases to the no-with rule.#16791
graphite-app[bot] merged 1 commit intomainfrom
add-more-tests-for-no-with

Conversation

@connorshea
Copy link
Member

Only one test case is a bit... slim.

@connorshea connorshea requested a review from camc314 as a code owner December 13, 2025 09:37
Copilot AI review requested due to automatic review settings December 13, 2025 09:37
@github-actions github-actions bot added A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Dec 13, 2025
Copy link

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

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

Test coverage improved substantially for non-violations, but the violation coverage remains thin: only one with-statement variant is asserted. Adding a few representative failing cases would better protect against regressions in parsing/AST matching across formatting and nesting patterns.

Summary of changes

What changed

  • Expanded the no-with rule test coverage in crates/oxc_linter/src/rules/eslint/no_with.rs by replacing a single passing fixture ("foo.bar()") with a broader pass suite.
  • Added multiple non-with-statement scenarios that include the token with in:
    • comments (/* ... */, // ...)
    • identifiers/properties (obj.with, { with: 1 }, ['with'], destructuring { with: w })
    • class methods (with() {})
    • string/template literals ('with in string', `with in template`)
  • Left the failing fixture as a single canonical with(foo) { bar() } case.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances test coverage for the no-with ESLint rule by adding 11 new test cases to the existing single test case. The new tests comprehensively verify that the rule correctly allows the keyword "with" in contexts where it's not used as a statement (comments, strings, property names, method names, destructuring).

Key Changes:

  • Expanded the pass test vector from 1 to 12 test cases
  • Added edge cases covering comments, strings, template literals, property access, methods, computed properties, and destructuring

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 13, 2025

CodSpeed Performance Report

Merging #16791 will not alter performance

Comparing add-more-tests-for-no-with (d2850a9) with main (0d0d606)

Summary

✅ 4 untouched
⏩ 41 skipped1

Footnotes

  1. 41 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@connorshea connorshea added the 0-merge Merge with Graphite Merge Queue label Dec 13, 2025
Copy link
Member Author

connorshea commented Dec 13, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the add-more-tests-for-no-with branch from d2850a9 to 710c3a7 Compare December 13, 2025 09:47
@graphite-app graphite-app bot merged commit 710c3a7 into main Dec 13, 2025
20 checks passed
@graphite-app graphite-app bot deleted the add-more-tests-for-no-with branch December 13, 2025 09:52
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants