refactor(tests): replace @ts-ignore with @ts-expect-error for improved type safety #4825
Conversation
Varadraj75
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaced occurrences of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
hey @princerajpoot20 @anshgoyalevil , please review and merge the pr |
There was a problem hiding this comment.
Pull request overview
This PR refactors test files to replace @ts-ignore directives with @ts-expect-error for better type safety. The change ensures that TypeScript will error if the suppressed type error is fixed, preventing outdated suppressions from lingering in the codebase. Each replacement includes an explanatory comment describing why the type error is expected.
Key changes:
- Replaced all 21 occurrences of
@ts-ignorewith@ts-expect-erroracross 6 test files - Added descriptive comments explaining the intentional type violations being tested
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tools/tools-object.test.ts | Updated 4 type suppressions for testing wrong data types, unknown categories, and malformed YAML |
| tests/tools/combine-tools.test.ts | Updated 1 type suppression for testing missing required properties |
| tests/markdown/check-markdown.test.ts | Updated 4 type suppressions for testing invalid and missing frontmatter scenarios |
| tests/dashboard/build-dashboard.test.ts | Updated 3 type suppressions for testing missing function arguments and environment variables |
| tests/build-post-list.test.ts | Updated 8 type suppressions for testing undefined parameters and invalid input types |
| tests/build-docs/addDocButtons.test.ts | Updated 1 type suppression for testing invalid tree posts parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4825 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 798 798
Branches 146 146
=========================================
Hits 798 798 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4825--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/markdown/check-markdown.test.ts (1)
177-178: Consider clarifying the comment.The comment says "simulating valid frontmatter" but the test passes
undefinedtovalidateBlogsand expects the error "Frontmatter is missing". Consider updating the comment to "simulating missing frontmatter" or "testing undefined frontmatter" for clarity.🔎 Suggested comment update
- // @ts-expect-error – simulating valid frontmatter + // @ts-expect-error – testing undefined frontmatter const errors = validateBlogs(undefined);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/build-docs/addDocButtons.test.tstests/build-post-list.test.tstests/dashboard/build-dashboard.test.tstests/markdown/check-markdown.test.tstests/tools/combine-tools.test.tstests/tools/tools-object.test.ts
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:138-143
Timestamp: 2025-03-23T12:42:49.503Z
Learning: For the locale validation function in check-locales.ts, the team has decided that modifying the outer scope variable 'hasErrors' inside the forEach loop is acceptable despite the ESLint warning about unsafe references in loop functions.
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
📚 Learning: 2024-11-10T18:16:35.551Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:28-46
Timestamp: 2024-11-10T18:16:35.551Z
Learning: In the `scripts/build-post-list.js` JavaScript file, tests rely on the `result` object being a global variable. Moving it inside the `buildPostList` function causes tests to fail; therefore, keep `result` as a global variable.
Applied to files:
tests/build-post-list.test.ts
📚 Learning: 2024-11-01T09:55:20.531Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/build-rss.test.js:25-27
Timestamp: 2024-11-01T09:55:20.531Z
Learning: In `tests/build-rss.test.js`, replacing `jest.resetModules()` with `jest.resetAllMocks()` in the `afterEach()` block causes errors. It is necessary to use `jest.resetModules()` to reset the module registry between tests in this file.
Applied to files:
tests/build-post-list.test.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
tests/build-post-list.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Applied to files:
tests/build-post-list.test.tstests/tools/combine-tools.test.tstests/dashboard/build-dashboard.test.tstests/tools/tools-object.test.tstests/markdown/check-markdown.test.tstests/build-docs/addDocButtons.test.ts
📚 Learning: 2024-11-01T13:32:15.472Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/fixtures/rssData.js:1-57
Timestamp: 2024-11-01T13:32:15.472Z
Learning: In the `tests/fixtures/rssData.js` file of the `asyncapi/website` project, tests for edge cases such as empty strings for title or excerpt, very long text content, international characters (UTF-8), or malformed URLs in `slug` or `cover` are not necessary because these cases will not occur.
Applied to files:
tests/build-post-list.test.tstests/tools/tools-object.test.tstests/markdown/check-markdown.test.ts
📚 Learning: 2025-02-16T12:57:24.575Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-16T12:57:24.575Z
Learning: Tests failing after dependency upgrades indicate potential breaking changes and might require code changes rather than test modifications to support newer versions.
Applied to files:
tests/build-post-list.test.tstests/tools/combine-tools.test.tstests/dashboard/build-dashboard.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Applied to files:
tests/build-post-list.test.tstests/tools/combine-tools.test.tstests/dashboard/build-dashboard.test.tstests/tools/tools-object.test.tstests/markdown/check-markdown.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
Applied to files:
tests/build-post-list.test.tstests/tools/combine-tools.test.tstests/dashboard/build-dashboard.test.tstests/tools/tools-object.test.tstests/markdown/check-markdown.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/build-post-list.test.tstests/tools/combine-tools.test.tstests/dashboard/build-dashboard.test.tstests/tools/tools-object.test.tstests/markdown/check-markdown.test.ts
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
tests/build-post-list.test.tstests/dashboard/build-dashboard.test.tstests/markdown/check-markdown.test.tstests/build-docs/addDocButtons.test.ts
📚 Learning: 2024-11-29T19:42:31.175Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: scripts/tools/tools-object.js:93-98
Timestamp: 2024-11-29T19:42:31.175Z
Learning: In `scripts/tools/tools-object.js`, when validation fails in the `convertTools` function, errors should be thrown or rejected instead of only logging them, to ensure proper error propagation and handling.
Applied to files:
tests/tools/combine-tools.test.tstests/tools/tools-object.test.ts
📚 Learning: 2024-11-01T12:49:32.805Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3136
File: scripts/tools/combine-tools.js:122-125
Timestamp: 2024-11-01T12:49:32.805Z
Learning: In `scripts/tools/combine-tools.js`, the existing URL parsing logic for `repoUrl` without additional error handling is acceptable and does not require changes.
Applied to files:
tests/tools/combine-tools.test.ts
📚 Learning: 2024-12-02T05:52:49.547Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: tests/helper/toolsObjectData.js:1-18
Timestamp: 2024-12-02T05:52:49.547Z
Learning: In `tests/helper/toolsObjectData.js`, using the hard-coded repository ID directly in the URL is acceptable when creating mock data for tests, as we are not asserting anything at `REPO_ID`.
Applied to files:
tests/tools/tools-object.test.ts
📚 Learning: 2024-11-25T18:34:51.303Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Applied to files:
tests/markdown/check-markdown.test.ts
📚 Learning: 2024-10-18T17:24:45.053Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:24:45.053Z
Learning: In `scripts/markdown/check-markdown.js`, the script is not run in an asynchronous environment, so refactoring it to use `async/await` is unnecessary.
Applied to files:
tests/markdown/check-markdown.test.ts
📚 Learning: 2024-12-30T11:00:42.064Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: jest.config.cjs:6-7
Timestamp: 2024-12-30T11:00:42.064Z
Learning: The user only wants coverage for scripts, not for .tsx files, because the existing tests are focused on scripts.
Applied to files:
tests/markdown/check-markdown.test.ts
📚 Learning: 2024-10-18T17:37:50.602Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:31-31
Timestamp: 2024-10-18T17:37:50.602Z
Learning: In `scripts/markdown/check-markdown.js`, the `frontmatter` object cannot have a property named `hasOwnProperty`, so using `frontmatter.hasOwnProperty(attr)` is acceptable.
Applied to files:
tests/markdown/check-markdown.test.ts
📚 Learning: 2025-04-20T16:05:16.482Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Applied to files:
tests/markdown/check-markdown.test.ts
🧬 Code graph analysis (2)
tests/build-post-list.test.ts (1)
scripts/build-post-list.ts (3)
buildPostList(288-314)addItem(74-90)slugifyToC(39-51)
tests/dashboard/build-dashboard.test.ts (1)
scripts/dashboard/build-dashboard.ts (2)
writeToFile(329-329)getDiscussionByID(322-322)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: cypress-run
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-latest
- GitHub Check: Lighthouse CI
🔇 Additional comments (7)
tests/build-post-list.test.ts (1)
203-298: LGTM! Type safety improvements applied consistently.The replacements of
@ts-ignorewith@ts-expect-errorthroughout this file are appropriate. The descriptive comments clearly explain why each type error is intentionally expected (testing invalid parameters, type violations, etc.). This change will cause TypeScript to fail if any of these errors no longer exist, making the suppressions more maintainable.tests/build-docs/addDocButtons.test.ts (1)
84-84: LGTM! Improved type safety directive.The replacement is appropriate and the comment clearly explains that this test intentionally passes an invalid data structure to verify error handling.
tests/tools/combine-tools.test.ts (1)
290-290: LGTM! Type safety improvement for missing properties test.The replacement is appropriate and the comment clearly explains that this test intentionally passes tools without required properties to verify error handling.
tests/tools/tools-object.test.ts (1)
76-175: LGTM! Type safety improvements applied consistently.All four replacements of
@ts-ignorewith@ts-expect-errorare appropriate. The descriptive comments clearly explain the test scenarios: testing wrong data types, unknown categories, and malformed JSON handling. This ensures TypeScript will fail if these intentional errors no longer exist.tests/markdown/check-markdown.test.ts (2)
59-86: LGTM! Type safety improvements for invalid frontmatter tests.The replacements are appropriate and comments clearly explain that these tests simulate invalid or missing frontmatter scenarios.
185-185: LGTM! Type safety improvement for valid frontmatter test.The replacement is appropriate. The comment accurately describes that this tests valid frontmatter (ensuring no errors are returned).
tests/dashboard/build-dashboard.test.ts (1)
262-273: LGTM! Type safety improvements for error handling tests.All three replacements of
@ts-ignorewith@ts-expect-errorare appropriate. The descriptive comments clearly explain the test scenarios: intentionally triggering write failures and testing missing environment variables. This ensures TypeScript will fail if these intentional errors no longer exist.
|
Hi @Varadraj75, I have submitted my PR #4826 |
|
closing in support of #4825 (comment) |
4 participants
Description
@ts-ignoreusages in test files with@ts-expect-error.Related issue(s)
Fixes #4797
Verification
@ts-ignoreusages remain (grep -R "@ts-ignore" tests).Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.