test(linter/plugins): remove @ts-expect-error from tests#16795
Conversation
github-actions
bot
added
A-linter
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main remaining issue is that several places replace TS suppressions with assertions, but still rely on unchecked indexing (declarations[0]) and non-null assertions (init!), which undermines the goal of making test intent explicit. Adding a couple of targeted assertions (array non-empty, init != null) would make failures clearer and more robust. There’s also some repeated AST-unwrapping logic in the JSX tests that could be factored into a small helper for maintainability.
Summary of changes
What changed
apps/oxlint/test/fixtures/sourceCode/plugin.ts
- Replaced
// @ts-ignore-based property access with runtime assertions to narrow AST node types. - Extracted
ast.body[0]intostmt, assertedVariableDeclaration, then assertedidis anIdentifierand usedid.namein the reported message.
apps/oxlint/test/isSpaceBetween.test.ts
- Added
node:assertand switched fixture AST typing from a genericNode[]cast toProgram. - Replaced
// @ts-expect-erroraccesses with explicit structural assertions (stmt.type,jsx.type,closingElement !== null, non-null first/last nodes/tokens). - Expanded coverage by asserting
isSpaceBetween*behavior for both argument orders (normal and reversed). - Minor test description grammar fix ("either" → "if either").
There was a problem hiding this comment.
Pull request overview
This PR improves type safety in test files by removing @ts-expect-error and @ts-ignore comments and replacing them with explicit runtime assertions using node:assert. This ensures that test inputs are validated to be what the tests expect them to be, while also providing TypeScript with proper type narrowing information that expect assertions cannot provide.
Key Changes:
- Replace type suppression comments with explicit
assertandassert.strictEqualchecks - Add proper type assertions for AST nodes (checking types like "VariableDeclaration", "Identifier", "JSXElement")
- Extract values into named variables before assertions for better readability
- Add test cases for reversed argument order in
isSpaceBetweentests
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/oxlint/test/isSpaceBetween.test.ts | Replaced @ts-expect-error comments with explicit assertions for null checks, type checks, and added reversed-order test cases for thoroughness |
| apps/oxlint/test/fixtures/sourceCode/plugin.ts | Replaced @ts-ignore comments with type assertions and extracted AST node properties into variables with proper type validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge activity
|
Remove `// @ts-expect-error` and `// @ts-ignore` comments from test fixtures, and use assertions instead. This makes sure that the input to methods under test is what we intend it to be. Note: Using `assert` rather than `expect`, because `assert` gives TypeScript information about types, which `expect` doesn't seem to be capable of.
graphite-app
bot
force-pushed
the
12-13-test_linter_plugins_remove_defunct_test_case
branch
from
be35cb1 to
7277d4c
Compare
graphite-app
bot
force-pushed
the
12-13-test_linter_plugins_remove_ts-expect-error_from_tests
branch
from
151fe2b to
26ae895
Compare
graphite-app
bot
deleted the
12-13-test_linter_plugins_remove_ts-expect-error_from_tests
branch
Remove
// @ts-expect-errorand// @ts-ignorecomments from test fixtures, and use assertions instead. This makes sure that the input to methods under test is what we intend it to be.Note: Using
assertrather thanexpect, becauseassertgives TypeScript information about types, whichexpectdoesn't seem to be capable of.