Skip to content

Conversation

@singhaditya73
Copy link
Contributor

@singhaditya73 singhaditya73 commented Dec 31, 2025

This PR replaces @ts-ignore with @ts-expect-error in test files to improve type safety. It ensures TypeScript fails when an expected error no longer exists, preventing outdated suppressions.


🎯 Reason

Tests sometimes intentionally pass invalid data, where TypeScript errors are expected.

Why @ts-expect-error?

  • Fails if the suppressed error is fixed
  • Prevents silent, outdated suppressions
  • Encourages descriptive comments
  • Recommended by TypeScript

📋 Changes

  • Updated 22 instances across multiple test files
  • Covered invalid parameters, missing arguments, and incorrect data types

✅ Example

Before

// @ts-ignore
const result = processData(invalidInput);

After

// @ts-expect-error - intentional invalid input
const result = processData(invalidInput);

🔍 Verification

Screenshot showing no @ts-ignore remaining in the tests folder:
Screenshot 2025-12-31 231350

Summary by CodeRabbit

  • Tests
    • Enhanced TypeScript error handling annotations in test suite to enforce explicit compile-time validation of expected errors.

✏️ Tip: You can customize this high-level summary in your review settings.

…d type safety

Replace all 22 @ts-ignore directives with @ts-expect-error across 6 test files. Each replacement includes a descriptive comment explaining the intentional type violation being tested. This ensures TypeScript will error if the suppressed type issue is fixed, preventing outdated suppressions from lingering.

Closes asyncapi#4797
@netlify
Copy link

netlify bot commented Dec 31, 2025

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 0b4de1e
🔍 Latest deploy log https://app.netlify.com/projects/asyncapi-website/deploys/695563c76be512000894ef5c
😎 Deploy Preview https://deploy-preview-4826--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 31, 2025

📝 Walkthrough

Walkthrough

Replace TypeScript error suppression directives from // @ts-ignore to `// `@ts-expect-error across six test files to provide explicit error expectation annotations. No runtime behavior changes; compile-time type-check directive updates only.

Changes

Cohort / File(s)
TypeScript Test Annotation Updates
tests/build-docs/addDocButtons.test.ts, tests/build-post-list.test.ts, tests/dashboard/build-dashboard.test.ts, tests/markdown/check-markdown.test.ts, tests/tools/combine-tools.test.ts, tests/tools/tools-object.test.ts

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related issues

Poem

🐰 From ignore to expect with care,
Type errors now declared so fair,
No logic bent, no flow astray,
Just clearer intent, hip-hop hooray!
~Thump-thump 🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: replacing @ts-ignore with @ts-expect-error across test files for type safety improvements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2747eba and 872945e.

📒 Files selected for processing (6)
  • tests/build-docs/addDocButtons.test.ts
  • tests/build-post-list.test.ts
  • tests/dashboard/build-dashboard.test.ts
  • tests/markdown/check-markdown.test.ts
  • tests/tools/combine-tools.test.ts
  • tests/tools/tools-object.test.ts
🧰 Additional context used
🧠 Learnings (16)
📓 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.
📚 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/dashboard/build-dashboard.test.ts
  • tests/markdown/check-markdown.test.ts
  • tests/build-docs/addDocButtons.test.ts
  • tests/tools/tools-object.test.ts
  • tests/build-post-list.test.ts
  • tests/tools/combine-tools.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/markdown/check-markdown.test.ts
  • tests/build-docs/addDocButtons.test.ts
  • tests/tools/tools-object.test.ts
  • tests/build-post-list.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: 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/markdown/check-markdown.test.ts
  • tests/build-docs/addDocButtons.test.ts
  • tests/build-post-list.test.ts
  • tests/tools/combine-tools.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: 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/markdown/check-markdown.test.ts
  • tests/build-docs/addDocButtons.test.ts
  • tests/tools/tools-object.test.ts
  • tests/build-post-list.test.ts
  • tests/tools/combine-tools.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/markdown/check-markdown.test.ts
  • tests/build-docs/addDocButtons.test.ts
  • tests/tools/tools-object.test.ts
  • tests/build-post-list.test.ts
  • tests/tools/combine-tools.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: 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/markdown/check-markdown.test.ts
  • tests/tools/tools-object.test.ts
  • tests/build-post-list.test.ts
  • tests/tools/combine-tools.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
📚 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/tools/tools-object.test.ts
  • tests/build-post-list.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-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/tools-object.test.ts
  • tests/tools/combine-tools.test.ts
📚 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-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
🧬 Code graph analysis (1)
tests/build-post-list.test.ts (1)
scripts/build-post-list.ts (3)
  • buildPostList (288-314)
  • addItem (74-90)
  • slugifyToC (39-51)
⏰ 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). (5)
  • GitHub Check: Redirect rules - asyncapi-website
  • GitHub Check: Header rules - asyncapi-website
  • GitHub Check: Pages changed - asyncapi-website
  • GitHub Check: Test NodeJS PR - windows-latest
  • GitHub Check: Lighthouse CI
🔇 Additional comments (8)
tests/tools/combine-tools.test.ts (1)

129-130: LGTM! Improved type safety with @ts-expect-error.

The replacement of @ts-ignore with @ts-expect-error is a best practice improvement. The directive will now cause TypeScript compilation to fail if the expected error no longer occurs, preventing outdated suppressions. The descriptive comments clearly explain why the type violation is intentional.

Also applies to: 290-290

tests/build-docs/addDocButtons.test.ts (1)

84-85: LGTM! Appropriate use of @ts-expect-error.

The change correctly uses @ts-expect-error to explicitly expect a TypeScript error when intentionally passing an invalid treePosts structure. The descriptive comment makes the test intent clear.

tests/tools/tools-object.test.ts (1)

76-76: LGTM! Proper error expectation annotations.

All four replacements appropriately use @ts-expect-error with clear, descriptive comments that explain the intentional type violations being tested (missing optional parameters, unknown categories, malformed content). This improves type safety by ensuring these suppressions remain valid.

Also applies to: 98-99, 147-148, 175-175

tests/markdown/check-markdown.test.ts (1)

59-60: LGTM! Enhanced type safety in validation tests.

The four replacements correctly use @ts-expect-error to test frontmatter validation with intentionally invalid data (wrong types, undefined, minimal data). The descriptive comments make each test's purpose clear and the change aligns with TypeScript best practices.

Also applies to: 86-87, 177-178, 185-186

tests/dashboard/build-dashboard.test.ts (1)

262-263: LGTM! Correct error expectations for missing arguments.

The three replacements appropriately use @ts-expect-error to test error handling when functions are called without required arguments (writeToFile, getDiscussionByID, getDiscussions). The descriptive comments clearly document the test intent.

Also applies to: 270-273

tests/build-post-list.test.ts (3)

203-206: LGTM! Proper error expectations for undefined parameters.

The replacements correctly use @ts-expect-error to test error handling when required parameters (basePath, writeFilePath) are intentionally undefined. The descriptive comments make the test intent explicit.

Also applies to: 210-213


219-226: LGTM! Comprehensive invalid input testing.

The four replacements appropriately use @ts-expect-error to test validation of various invalid details objects (null, empty object, invalid slug type, undefined). The descriptive comments clearly document each test case.


294-299: LGTM! Graceful handling of invalid input types.

The three replacements correctly use @ts-expect-error to verify that slugifyToC handles invalid input types gracefully (null, undefined, number). The descriptive comments make the defensive programming test cases clear.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Dec 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (2747eba) to head (0b4de1e).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #4826   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          798       798           
  Branches       146       146           
=========================================
  Hits           798       798           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Dec 31, 2025

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 38
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-4826--asyncapi-website.netlify.app/

@singhaditya73
Copy link
Contributor Author

@coderabbitai review

Please verify if this PR is ready to merge. All CI checks have passed (tests on Ubuntu, Windows, macOS) and the code coverage remains at 100%.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 31, 2025

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if any @ts-ignore remains in test files
echo "=== Checking for remaining @ts-ignore in tests folder ==="
rg -n '@ts-ignore' tests/ || echo "No @ts-ignore found in tests folder"

echo -e "\n=== Count of @ts-expect-error in tests folder ==="
rg -c '@ts-expect-error' tests/ | wc -l

echo -e "\n=== Sample of @ts-expect-error usage with context ==="
rg -n -A1 '@ts-expect-error' tests/ | head -30

echo -e "\n=== Check git diff stats ==="
git diff origin/master --stat tests/

echo -e "\n=== Sample of actual changes in one file ==="
git diff origin/master tests/build-post-list.test.ts | head -60

Length of output: 6315


@singhaditya73: I've verified your PR and it looks excellent! ✅

Verification Results:

  • No @ts-ignore remains in the tests folder
  • All 22 replacements completed across 6 test files with descriptive comments
  • ✅ Each @ts-expect-error includes clear explanations (e.g., "Intentionally passing undefined basePath to test error handling")
  • ✅ Changes are test-only, no production code affected
  • ✅ All CI checks passed (Ubuntu, Windows, macOS)
  • ✅ Code coverage remains at 100%

Example of improvements:

// Before:
// @ts-ignore, ignore the type error for this test
await expect(buildPostList(postDirectories, undefined, writeFilePath))...

// After:
// @ts-expect-error - Intentionally passing undefined basePath to test error handling
await expect(buildPostList(postDirectories, undefined, writeFilePath))...

This refactoring follows TypeScript best practices and improves type safety by ensuring TypeScript will fail if the expected errors no longer exist, preventing outdated suppressions.

The PR is ready to merge! 🎉


🧠 Learnings used
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch

Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: tests/fixtures/toolsObjectData.js:51-52
Timestamp: 2024-10-09T17:35:36.557Z
Learning: When reviewing code in the 'asyncapi/website' repository, akshatnema prefers that I do not provide committable code suggestions.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@asyncapi-bot
Copy link
Contributor

@derberg @princerajpoot20 @akshatnema @sambhavgupta0705 @anshgoyalevil @Mayaleeeee Please take a look at this PR. Thanks! 👋

@singhaditya73
Copy link
Contributor Author

hey @princerajpoot20 @anshgoyalevil could u please review this PR.
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants