Skip to content

chore(linter): port additional tests from unicorn/prefer-at rule.#17632

Merged
graphite-app[bot] merged 1 commit into
mainfrom
prefer-at-test-cases
Jan 4, 2026
Merged

chore(linter): port additional tests from unicorn/prefer-at rule.#17632
graphite-app[bot] merged 1 commit into
mainfrom
prefer-at-test-cases

Conversation

@connorshea

Copy link
Copy Markdown
Member

We were missing various tests that are generated dynamically in the upstream tests, unfortunately most of them fail currently. I am unsure if we excluded these test cases intentionally when adding this rule, but I assume not? They don't get picked up by the rulegen logic due to being dynamically generated.

See https://github.com/sindresorhus/eslint-plugin-unicorn/blob/8fcae1aa5a3f1f2c6014bf33aeda2f016c0a8d9a/test/prefer-at.js for further context.

I copy-pasted these tests over, and then commented out the new ones that failed.

Copilot AI review requested due to automatic review settings January 4, 2026 17:41
@connorshea connorshea requested a review from camc314 as a code owner January 4, 2026 17:41
@github-actions github-actions Bot added A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Jan 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 ports additional test cases from the upstream eslint-plugin-unicorn prefer-at rule that were not previously included. The tests are dynamically generated in the upstream project and were not picked up by the rulegen logic. Some of the newly added test cases are commented out because they currently fail.

  • Added 9 new passing test cases for checkAllIndexAccess: true option
  • Added 2 new failing test cases that currently pass (tests for array[1] and array[array.length - 1])
  • Commented out 13 additional failing test cases to be fixed later
  • Fixed indentation in existing multi-line test strings to use consistent spacing

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
crates/oxc_linter/src/rules/unicorn/prefer_at.rs Added new test cases for checkAllIndexAccess option, including passing tests and failing tests (some commented out); fixed indentation in multi-line test strings
crates/oxc_linter/src/snapshots/unicorn_prefer_at.snap Updated snapshot with new test outputs for the two uncommented failing tests and adjusted line numbers to reflect indentation changes

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

Comment thread crates/oxc_linter/src/rules/unicorn/prefer_at.rs Outdated
@codspeed-hq

codspeed-hq Bot commented Jan 4, 2026

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #17632 will not alter performance

Comparing prefer-at-test-cases (c8016f9) with main (25317c9)

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.

@camc314 camc314 changed the title chore(linter): Port additional tests from unicorn/prefer-at rule. chore(linter): port additional tests from unicorn/prefer-at rule. Jan 4, 2026
@camc314 camc314 self-assigned this Jan 4, 2026
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Jan 4, 2026

camc314 commented Jan 4, 2026

Copy link
Copy Markdown
Contributor

Merge activity

…7632)

We were missing various tests that are generated dynamically in the upstream tests, unfortunately most of them fail currently. I am unsure if we excluded these test cases intentionally when adding this rule, but I assume not? They don't get picked up by the rulegen logic due to being dynamically generated.

See https://github.com/sindresorhus/eslint-plugin-unicorn/blob/8fcae1aa5a3f1f2c6014bf33aeda2f016c0a8d9a/test/prefer-at.js for further context.

I copy-pasted these tests over, and then commented out the new ones that failed.
@graphite-app graphite-app Bot force-pushed the prefer-at-test-cases branch from c8016f9 to 431ca00 Compare January 4, 2026 19:45
@graphite-app graphite-app Bot merged commit 431ca00 into main Jan 4, 2026
20 checks passed
@graphite-app graphite-app Bot deleted the prefer-at-test-cases branch January 4, 2026 19:51
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Jan 4, 2026
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.

3 participants