Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add tests for tool-object script #3265

Merged
merged 74 commits into from
Dec 7, 2024

Conversation

vishvamsinh28
Copy link
Contributor

@vishvamsinh28 vishvamsinh28 commented Sep 30, 2024

This PR adds tests for tools-object.js script

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced error handling and formatting for tool management functions.
    • Introduced utility functions for generating mock data related to tools.
  • Bug Fixes

    • Improved error management in the tool conversion process, ensuring better logging and descriptive error messages.
  • Tests

    • Added comprehensive unit tests for tool management functions, covering creation, conversion, category assignment, and error handling.
    • Introduced mock data utilities to facilitate testing scenarios.

Copy link

netlify bot commented Sep 30, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9427070
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/67547c0e3f1c89000876dfa8
😎 Deploy Preview https://deploy-preview-3265--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 site configuration.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Sep 30, 2024

⚡️ Lighthouse report for the changes in this PR:

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

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

@anshgoyalevil anshgoyalevil added the gsoc This label should be used for issues or discussions related to ideas for Google Summer of Code label Sep 30, 2024
Copy link

coderabbitai bot commented Oct 4, 2024

Walkthrough

The pull request introduces enhancements to the scripts/tools/tools-object.js file, focusing on error handling and code formatting for the createToolObject and convertTools functions. It restructures the error management within convertTools, implementing a try-catch block to capture and log errors. Additionally, a comprehensive test suite is added in tests/tools/tools-object.test.js to validate the functionalities of these functions, including error handling scenarios. A new helper file, tests/helper/toolsObjectData.js, is also created to provide mock data structures for testing purposes.

Changes

File Path Change Summary
scripts/tools/tools-object.js Enhanced error handling in convertTools, standardized parameter formatting in createToolObject, and reformatted export statement for consistency.
tests/tools/tools-object.test.js Introduced a comprehensive suite of unit tests for convertTools and createToolObject, validating functionalities and error handling with Jest and Axios mocks.
tests/helper/toolsObjectData.js Added utility functions for generating mock data structures related to tools, including repository data and tool file content.

Possibly related PRs

Suggested labels

ready-to-merge

Suggested reviewers

  • derberg
  • akshatnema
  • magicmatatjahu
  • sambhavgupta0705
  • Mayaleeeee
  • devilkiller-ag
  • anshgoyalevil
  • asyncapi-bot-eve

🐰 In the code, we hop and play,
With tools and tests, we pave the way.
Errors caught, like carrots in a patch,
Mock data ready, no need to scratch.
So let’s rejoice, with a joyful cheer,
For smoother coding, the end is near! 🥕


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (15)
scripts/tools/tools-object.js (3)

50-52: LGTM: Improved error handling with try-catch block

The addition of the try-catch block around the entire function body significantly improves error handling. Moving the initializations inside the try block ensures that any errors during these steps are caught as well.

One minor suggestion:

Consider adding more specific error handling by creating custom error types or adding more context to the error message. This could help with debugging and error tracking in the future.

Example:

} catch (err) {
  throw new Error(`Error processing tools: ${err.message}. Stack: ${err.stack}`);
}

Also applies to: 111-114


63-108: LGTM: Improved inner try-catch block and error handling

The flattened structure of the inner try-catch block improves readability and maintainability. The improved error logging provides more context for debugging, which is excellent.

One suggestion for further improvement:

Consider using a more descriptive variable name instead of err in the catch block. This can provide more context about the type of error being caught.

Example:

} catch (toolProcessingError) {
  console.error(toolProcessingError);
  throw toolProcessingError;
}

Line range hint 1-117: Overall assessment: Significant improvements in error handling and code quality

The changes made to this file represent a substantial improvement in error handling, code formatting, and overall readability. Key improvements include:

  1. Enhanced error handling with try-catch blocks
  2. Improved function signatures with default parameters
  3. Standardized code formatting and indentation
  4. Better error logging for debugging purposes

These changes will contribute to better maintainability and robustness of the codebase. Great job on these improvements!

For future enhancements, consider:

  1. Implementing more granular error handling with custom error types
  2. Adding unit tests to cover the error handling scenarios introduced in this update
  3. Documenting the error handling approach for other developers working on this file
tests/tools/tools-object.test.js (5)

36-53: LGTM: Well-structured test suite setup and initial test cases.

The test suite is properly set up with a describe block and beforeEach for mock clearing. The first two test cases for createToolObject cover basic functionality well.

Consider adding test cases for edge cases, such as:

  1. When both toolFile.description and repoDescription are undefined.
  2. When isAsyncAPIrepo is false.

55-72: LGTM: Comprehensive tests for convertTools function.

The test cases for the convertTools function cover important scenarios, including correct data conversion and assignment to the "Others" category. The use of mocked Axios responses and detailed assertions is commendable.

Consider adding the following assertions to strengthen the tests:

  1. In the first test (lines 55-62), verify that the correct URL is passed to axios.get.
  2. In the second test (lines 64-72), also check that the tool is not present in any other category.

74-90: LGTM: Good coverage of error handling for invalid tool files.

The test case for handling invalid .asyncapi-tool files is well-implemented. The use of console.error spy to check for logged errors is a good practice.

Consider the following improvements:

  1. Instead of just logging errors, consider throwing or returning an error object from the convertTools function. This would allow for more robust error handling in the application code.
  2. Add a test case to verify that the function continues processing other tools even when one tool file is invalid.

117-137: LGTM: Good coverage of network and JSON validation errors.

The test cases for handling network errors and JSON schema validation failures are well-implemented. They cover critical error scenarios that could occur in real-world usage.

Consider adding the following error scenarios:

  1. Test case for a partially valid JSON (e.g., missing some required fields but otherwise valid).
  2. Test case for a timeout error from Axios.
  3. Test case for an unexpected HTTP status code (e.g., 404 or 500) from the API.

139-177: LGTM: Comprehensive testing of JSON parsing and property validation.

The test cases for handling invalid JSON format and missing required tool properties are well-implemented. They cover important error scenarios related to data parsing and validation.

Consider the following improvements:

  1. Group related test cases together. For example, move these JSON parsing and property validation tests closer to the other JSON schema validation test (around line 127).
  2. Consider using beforeEach to reset mocks for each test, especially for the utils module mock, to ensure test isolation.
  3. Add a test case for a scenario where some optional properties are missing but all required properties are present.
tests/fixtures/toolsObjectData.js (7)

19-53: LGTM: Well-defined test data for different stages of tool processing

The constants mockToolFileContent, toolFileT1, and expectedObjectT1 provide a clear representation of the different stages in processing a tool file. They are consistent and well-structured.

Consider adding a comment explaining the relationship between these constants to improve readability.

You could add a comment like this before line 19:

// The following constants represent different stages of processing a tool file:
// 1. mockToolFileContent: Raw content of the tool file
// 2. toolFileT1: Parsed version of the tool file
// 3. expectedObjectT1: Final processed version with additional properties

59-80: LGTM: Well-defined test case for handling missing properties

The constants toolFileT2 and expectedObjectT2 provide a good test case for handling missing properties and default values during tool processing. They will be useful for ensuring robust parsing and processing of tool files.

For consistency with the previous test case, consider adding a comment explaining the purpose of these constants.

You could add a comment like this before line 59:

// The following constants represent a test case for handling missing properties:
// 1. toolFileT2: Parsed version of a tool file with some properties missing
// 2. expectedObjectT2: Expected final processed version with default values and additional properties

82-104: LGTM: Comprehensive test case for tool categorization

The expectedObjectT3 constant provides a comprehensive test case for a function that categorizes tools. It includes both a specific category and an "Others" category, which is good for testing edge cases.

To improve clarity, consider adding a comment explaining the purpose and structure of this constant.

You could add a comment like this before line 82:

// expectedObjectT3: Represents the expected output of a function that categorizes tools
// It includes a specific category (Category1) and an "Others" category for comprehensive testing

106-132: LGTM: Comprehensive test cases for handling unknown categories

The constants dataWithUnknownCategory, toolFileContent, dataWithUnknownCategoryOnce, and unknownToolFileContent provide thorough test cases for handling tools with unknown categories. They cover different aspects of this scenario, which is crucial for robust error handling.

To improve organization and readability, consider grouping these related constants together in the file.

You could move the dataWithUnknownCategoryOnce and unknownToolFileContent constants (currently at lines 204-243) to be adjacent to the other unknown category constants (after line 132). This would group related test cases together, making the file structure more logical and easier to navigate.

Also applies to: 204-243


134-161: LGTM: Comprehensive test cases for handling invalid tool data

The constants invalidToolFileContent, invalidToolData, invalidToolFileContentJSON, and invalidJsonContent provide a good range of test cases for handling various types of invalid tool data. This variety is crucial for ensuring robust error handling in the tool processing logic.

To improve organization and clarity:

  1. Consider grouping these related constants together in the file.
  2. The naming of invalidToolFileContent and invalidToolFileContentJSON is very similar and might be confusing. Consider renaming for clarity.

Suggested improvements:

  1. Move the invalidToolFileContentJSON and invalidJsonContent constants (currently at lines 245-261) to be adjacent to the other invalid tool constants (after line 161).
  2. Rename the constants for clarity, for example:
    • invalidToolFileContent -> invalidToolFileContentWithExtraField
    • invalidToolFileContentJSON -> invalidToolFileContentMalformedJSON
      This would group related test cases together and make the purpose of each constant clearer.

Also applies to: 245-261


263-268: LGTM: Good test case for handling missing properties

The missingToolPropertyContent constant provides a good test case for handling tool files with missing required properties. This is an important edge case to test.

For consistency with other constants in the file, consider using a JSON-like string format instead of YAML.

You could modify the constant to use a JSON-like string format, like this:

const missingToolPropertyContent = `
{
  "title": "Missing Property Tool",
  "description": "This tool is missing required properties",
  "links": {
    "repoUrl": "https://github.com/asyncapi/missing-property"
  }
}`;

This would make it consistent with the format used in other constants like mockToolFileContent.


1-292: Overall: Well-structured and comprehensive test fixtures with minor improvement opportunities

This file provides a robust set of test fixtures covering a wide range of scenarios for tool processing, including valid cases, edge cases, and error cases. The variety and completeness of the test data will enable thorough testing of the tool processing logic.

Some suggestions for minor improvements:

  1. Group related constants together (e.g., all constants related to unknown categories, all constants related to invalid data).
  2. Add comments to explain the purpose of groups of related constants.
  3. Ensure consistency in the format of string constants (e.g., using JSON-like strings consistently).
  4. Consider renaming some constants for clarity, especially where names are very similar.

These changes would further enhance the readability and maintainability of this test fixture file.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e56289b and e6a3505.

📒 Files selected for processing (3)
  • scripts/tools/tools-object.js (2 hunks)
  • tests/fixtures/toolsObjectData.js (1 hunks)
  • tests/tools/tools-object.test.js (1 hunks)
🔇 Additional comments (10)
scripts/tools/tools-object.js (3)

28-28: LGTM: Improved function signature with default parameters

The updated function signature with default parameter values enhances code readability and provides better flexibility when calling the function. This change aligns with best practices in JavaScript.


54-60: LGTM: Improved code formatting and readability

The standardized formatting throughout the convertTools function significantly improves code readability and maintainability. The consistent indentation and spacing make the code easier to understand and follow.

Also applies to: 85-98


117-117: LGTM: Improved export statement formatting

The reformatted export statement improves code consistency and readability. This change aligns with common JavaScript formatting practices.

tests/tools/tools-object.test.js (3)

1-34: LGTM: Imports and mock setup are well-structured.

The imports, mock data setup, and mocking of external dependencies (Axios and categoryList) are comprehensive and follow good testing practices. This setup provides a solid foundation for the test suite.


92-115: LGTM: Thorough testing of duplicate tools and category assignment.

The test cases for handling duplicate tools and ensuring single assignment to the "Others" category are well-implemented. They cover important edge cases in tool categorization and demonstrate good use of mocked responses.

The deduplication logic in the "Others" category test (lines 103-115) is particularly well done, ensuring that the same tool is not added multiple times.


1-178: Overall: Excellent test coverage with room for minor enhancements.

This test suite for the tools-object script is comprehensive and well-structured. It covers the main functionality, error scenarios, and edge cases. The use of mocking, particularly for Axios and the categoryList, is commendable.

To further improve the test suite:

  1. Consider adding more edge cases as suggested in the individual comments.
  2. Reorganize some tests to group related scenarios together.
  3. Ensure consistent use of beforeEach for mock resets across all tests.
  4. Consider adding integration tests that cover the interaction between convertTools and createToolObject functions.

Great job on creating a robust test suite!

tests/fixtures/toolsObjectData.js (4)

1-17: LGTM: Well-structured mock data for GitHub API response

The mockData constant provides a good representation of a GitHub API response for a tool. It includes all necessary properties and is well-formed, making it suitable for testing purposes.


55-57: LGTM: Useful constants for repository-specific tests

The constants repositoryUrl, repoDescription, and isAsyncAPIrepo provide specific values for testing repository-related functionality. They are consistent with the data in other constants and will be useful for targeted tests.


163-202: LGTM: Well-structured test cases for handling duplicate tools

The constants duplicateToolData and duplicateToolFileContent provide well-structured test cases for handling duplicate tool entries. The separation of API response data and file content allows for thorough testing of duplicate handling at different stages of processing.


270-292: LGTM: Comprehensive and well-organized module exports

The module exports are comprehensive, including all constants defined in the file. This provides maximum flexibility for importing and using these test fixtures in various test scenarios. The exports are well-organized and easy to read.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
tests/build-tools.test.js (1)

Line range hint 1-89: Consider reorganizing the test suite for better structure

The new test case integrates well with the existing structure. However, to improve the overall organization and readability of the test suite, consider the following suggestions:

  1. Group related test cases together using describe blocks.
  2. Move common setup and teardown logic into beforeAll and afterAll hooks within these describe blocks.
  3. Use more descriptive test case names to clearly indicate what each test is verifying.

Here's a suggested restructure:

describe('buildTools', () => {
  // ... (existing setup code)

  describe('Successful scenarios', () => {
    it('should extract, convert, combine tools, and write to file', async () => {
      // ... (existing test case)
    });
  });

  describe('Error handling', () => {
    it('should handle getData error', async () => {
      // ... (existing test case)
    });

    it('should handle file write errors for invalid paths', async () => {
      // ... (new test case)
    });
  });

  // ... (existing teardown code)
});

This structure:

  1. Groups test cases into logical sections.
  2. Makes it easier to add new test cases in the appropriate sections.
  3. Improves readability and maintainability of the test suite.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e6a3505 and 5000a47.

📒 Files selected for processing (3)
  • tests/build-tools.test.js (1 hunks)
  • tests/fixtures/toolsObjectData.js (1 hunks)
  • tests/tools/tools-object.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/tools/tools-object.test.js
🔇 Additional comments (9)
tests/build-tools.test.js (1)

Line range hint 1-89: Overall assessment: Good addition with room for improvement

The introduction of the new test case for handling file write errors is a valuable addition to the test suite. It improves the coverage of error scenarios and helps ensure the robustness of the buildTools function.

However, there are opportunities to enhance both the new test case and the overall structure of the test suite:

  1. The new test case could be more comprehensive in its assertions and cleanup.
  2. The overall test suite could benefit from better organization using describe blocks.

These improvements would lead to a more maintainable and thorough test suite. Despite these suggestions, the current implementation is functional and adds value to the codebase.

tests/fixtures/toolsObjectData.js (8)

49-51: Verify the value of 'hasCommercial' in 'expectedObjectT1'

In expectedObjectT1, the hasCommercial property is set to true. Ensure that this reflects the intended test case scenario and that it aligns with the corresponding tool file data.


77-78: Mismatch in 'hasCommercial' value in 'expectedObjectT2'

In expectedObjectT2, the hasCommercial property is set to false, whereas in expectedObjectT1, it is true. Verify whether this difference is intentional based on the test case specifications.


70-71: 'Description' field missing in 'toolFileT2' but present in 'expectedObjectT2'

In toolFileT2 (line 59~), there is no description field provided. However, expectedObjectT2 includes a description field sourced from repoDescription. Verify that your code is designed to default the description to the repository description when none is provided in the tool file.


106-122: Handle unknown categories in 'dataWithUnknownCategory'

The dataWithUnknownCategory object includes tools categorized under 'UnknownCategory'. Ensure that your tests check how the code handles tools with unknown categories and that it aligns with the expected behavior (e.g., assigning them to an 'Others' category or excluding them).


145-161: Test handling of invalid tool data in 'invalidToolData'

The invalidToolData represents a tool with an invalid field (invalidField). Confirm that your tests validate the schema properly and that your code correctly identifies and handles such invalid entries.


163-192: Ensure proper handling of duplicate tool entries

The duplicateToolData contains duplicate entries for the same tool. Verify that your code can detect duplicates and handles them according to the requirements, such as merging them, deduplicating, or raising an error.


245-254: Test parsing of malformed tool file content

The toolFileMalformedJSON constant contains malformed YAML content (missing a colon after filters). Ensure that your tests cover parsing errors and that your code gracefully handles exceptions thrown due to invalid file formats.


255-275: Export all defined constants

Review the module.exports object to confirm that all the defined constants are exported correctly. Missing exports could lead to tests failing due to undefined references.

tests/build-tools.test.js Outdated Show resolved Hide resolved
tests/fixtures/toolsObjectData.js Outdated Show resolved Hide resolved
tests/fixtures/toolsObjectData.js Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
tests/fixtures/toolsObjectData.js (2)

134-144: Add comments to clarify intentionally invalid test data

The invalidToolFileContent includes an invalidField within the filters section to simulate an invalid schema. Adding a comment to indicate that this is deliberate for testing invalid scenarios would improve readability and help other developers understand the intent.


245-254: Comment on malformed JSON for clarity

The toolFileMalformedJSON string is intentionally malformed to test error handling. Including a comment explaining that this JSON is malformed on purpose would aid in understanding and maintainability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5000a47 and 56b0fdd.

📒 Files selected for processing (2)
  • tests/fixtures/toolsObjectData.js (1 hunks)
  • tests/tools/tools-object.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/tools/tools-object.test.js
🧰 Additional context used

tests/fixtures/toolsObjectData.js Outdated Show resolved Hide resolved
tests/fixtures/toolsObjectData.js Outdated Show resolved Hide resolved
tests/fixtures/toolsObjectData.js Outdated Show resolved Hide resolved
tests/fixtures/toolsObjectData.js Outdated Show resolved Hide resolved
tests/tools/tools-object.test.js Outdated Show resolved Hide resolved
@vishvamsinh28
Copy link
Contributor Author

@akshatnema applied suggestions

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
scripts/tools/tools-object.js (1)

50-50: Consider using const for finalToolsObject.

Since finalToolsObject is only assigned once during initialization, it can be declared as const.

-    let finalToolsObject = {};
+    const finalToolsObject = {};
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bff29b8 and 7f7ce0d.

📒 Files selected for processing (1)
  • scripts/tools/tools-object.js (2 hunks)
🧰 Additional context used
🪛 eslint
scripts/tools/tools-object.js

[error] 55-55: Replace category with (category)

(prettier/prettier)


[error] 64-64: Insert ⏎······

(prettier/prettier)


[error] 65-65: Insert ··

(prettier/prettier)


[error] 66-66: Insert ··

(prettier/prettier)


[error] 67-67: Replace ·········· with ············

(prettier/prettier)


[error] 68-68: Insert ··

(prettier/prettier)


[error] 68-68: 'downloadUrl' is assigned a value but never used.

(no-unused-vars)


[error] 70-70: Replace ·········· with ············

(prettier/prettier)


[error] 72-72: Insert ··

(prettier/prettier)


[error] 72-72: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 73-73: Replace ··········const·jsonToolFileContent·=·await·convertToJson(toolFileContent) with ············const·jsonToolFileContent·=·await·convertToJson(toolFileContent);

(prettier/prettier)


[error] 75-75: Replace ·········· with ············

(prettier/prettier)


[error] 75-75: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 76-76: Replace ··········const·isValid·=·await·validate(jsonToolFileContent) with ············const·isValid·=·await·validate(jsonToolFileContent);

(prettier/prettier)


[error] 78-78: Insert ··

(prettier/prettier)


[error] 79-79: Insert ··

(prettier/prettier)


[error] 80-80: Insert ··

(prettier/prettier)


[error] 81-81: Replace const·isAsyncAPIrepo·=·tool.repository.owner.login·===·"asyncapi" with ··const·isAsyncAPIrepo·=·tool.repository.owner.login·===·'asyncapi'

(prettier/prettier)


[error] 82-82: Replace ············const·toolObject·=·await·createToolObject(jsonToolFileContent,·repositoryUrl,·repoDescription,·isAsyncAPIrepo with ··············const·toolObject·=·await·createToolObject(⏎················jsonToolFileContent,⏎················repositoryUrl,⏎················repoDescription,⏎················isAsyncAPIrepo⏎··············

(prettier/prettier)


[error] 82-82: This line has a length of 123. Maximum allowed is 120.

(max-len)


[error] 84-84: Insert ··

(prettier/prettier)


[error] 85-85: Replace await·Promise.all( with ··await·Promise.all(⏎················

(prettier/prettier)


[error] 86-86: Replace ·············· with ··················

(prettier/prettier)


[error] 87-87: Insert ····

(prettier/prettier)


[error] 88-88: Replace ·············· with ··················

(prettier/prettier)


[error] 88-88: Use object destructuring.

(prefer-destructuring)


[error] 89-89: Insert ····

(prettier/prettier)


[error] 90-90: Replace ················ with ····················

(prettier/prettier)


[error] 91-91: Insert ····

(prettier/prettier)


[error] 92-92: Replace ············}) with ················})⏎··············

(prettier/prettier)


[error] 93-93: Insert ··

(prettier/prettier)


[error] 94-94: Insert ··

(prettier/prettier)


[error] 95-95: Replace ············ with ··············

(prettier/prettier)


[error] 96-96: Insert ··

(prettier/prettier)


[error] 97-97: Replace ············ with ··············

(prettier/prettier)


[error] 98-98: Insert ··

(prettier/prettier)


[error] 99-99: Insert ··

(prettier/prettier)


[error] 100-100: Insert ··

(prettier/prettier)


[error] 101-101: Replace ········console.error(err) with ··········console.error(err);

(prettier/prettier)


[error] 102-102: Insert ··

(prettier/prettier)


[error] 103-103: Insert ··

(prettier/prettier)


[error] 104-104: Replace ····})) with ······})⏎····);

(prettier/prettier)


[error] 107-107: Insert ;

(prettier/prettier)


[error] 111-111: Insert ;

(prettier/prettier)

🔇 Additional comments (3)
scripts/tools/tools-object.js (3)

27-27: LGTM! Well-structured function signature.

The function signature is well-formatted with proper spacing around default parameters, making it more readable.


94-98: Previous suggestion about custom LogService is still applicable.

🧰 Tools
🪛 eslint

[error] 94-94: Insert ··

(prettier/prettier)


[error] 95-95: Replace ············ with ··············

(prettier/prettier)


[error] 96-96: Insert ··

(prettier/prettier)


[error] 97-97: Replace ············ with ··············

(prettier/prettier)


[error] 98-98: Insert ··

(prettier/prettier)


111-111: LGTM! Clean export syntax.

The export statement follows best practices with proper object shorthand notation and spacing.

🧰 Tools
🪛 eslint

[error] 111-111: Insert ;

(prettier/prettier)

scripts/tools/tools-object.js Show resolved Hide resolved
scripts/tools/tools-object.js Outdated Show resolved Hide resolved
scripts/tools/tools-object.js Outdated Show resolved Hide resolved
scripts/tools/tools-object.js Outdated Show resolved Hide resolved
scripts/tools/tools-object.js Outdated Show resolved Hide resolved
scripts/tools/tools-object.js Outdated Show resolved Hide resolved
tests/helper/toolsObjectData.js Outdated Show resolved Hide resolved
tests/helper/toolsObjectData.js Outdated Show resolved Hide resolved
tests/helper/toolsObjectData.js Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (4)
tests/helper/toolsObjectData.js (2)

1-18: Consider making repository ID configurable and improving URL safety.

The hardcoded repository ID (351453552) in the URL construction could limit reusability. Additionally, the path concatenation needs URL encoding for safety.

Consider this improvement:

 const createToolRepositoryData = ({
   name = '.asyncapi-tool',
   refId = '61855e7365a881e98c2fe667a658a0005753d873',
   owner = 'asyncapi',
   repoName = 'example-repo',
   description = 'Example repository',
-  path = '.asyncapi-tool'
+  path = '.asyncapi-tool',
+  repoId = '351453552'
 } = {}) => ({
   name,
-  url: `https://api.github.com/repositories/351453552/contents/${path}?ref=${refId}`,
+  url: `https://api.github.com/repositories/${repoId}/contents/${encodeURIComponent(path)}?ref=${refId}`,
   repository: {
     full_name: `${owner}/${repoName}`,
     html_url: `https://github.com/${owner}/${repoName}`,
     description,
     owner: { login: owner }
   },
   path
 });

76-76: Consider exporting createToolRepositoryData.

The createToolRepositoryData function might be useful for other tests that need to mock repository data directly.

-module.exports = { createToolFileContent, createExpectedToolObject, createMockData, createMalformedYAML };
+module.exports = { createToolRepositoryData, createToolFileContent, createExpectedToolObject, createMockData, createMalformedYAML };
scripts/tools/tools-object.js (2)

27-28: Add input validation for required parameters.

Consider adding validation for the toolFile parameter as it's required for creating the tool object.

 const createToolObject = async (toolFile, repositoryUrl = '', repoDescription = '', isAsyncAPIrepo = '') => {
+  if (!toolFile || !toolFile.title) {
+    throw new Error('Tool file must contain a title');
+  }
   const resultantObject = {

49-51: Enhance error message with more context.

The error message in the catch block could include more details about what was being processed when the error occurred.

 try {
   let finalToolsObject = {};
   const dataArray = data.items;
+  if (!dataArray?.length) {
+    throw new Error('No tools data provided');
+  }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7f7ce0d and dd54d8c.

📒 Files selected for processing (2)
  • scripts/tools/tools-object.js (2 hunks)
  • tests/helper/toolsObjectData.js (1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/tools/tools-object.js (1)
Learnt from: akshatnema
PR: asyncapi/website#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.
🪛 eslint
scripts/tools/tools-object.js

[error] 64-64: Insert ⏎······

(prettier/prettier)


[error] 65-65: Insert ··

(prettier/prettier)


[error] 66-66: Insert ··

(prettier/prettier)


[error] 67-67: Insert ··

(prettier/prettier)


[error] 68-68: Insert ··

(prettier/prettier)


[error] 70-70: Insert ··

(prettier/prettier)


[error] 72-72: Insert ··

(prettier/prettier)


[error] 72-72: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 73-73: Replace ··········const·jsonToolFileContent·=·await·convertToJson(toolFileContent) with ············const·jsonToolFileContent·=·await·convertToJson(toolFileContent);

(prettier/prettier)


[error] 75-75: Insert ··

(prettier/prettier)


[error] 75-75: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 76-76: Replace const·isValid·=·await·validate(jsonToolFileContent) with ··const·isValid·=·await·validate(jsonToolFileContent);

(prettier/prettier)


[error] 78-78: Insert ··

(prettier/prettier)


[error] 79-79: Insert ··

(prettier/prettier)


[error] 80-80: Insert ··

(prettier/prettier)


[error] 81-81: Insert ··

(prettier/prettier)


[error] 82-82: Replace ············const·toolObject·=·await·createToolObject(jsonToolFileContent,·repositoryUrl,·repoDescription,·isAsyncAPIrepo with ··············const·toolObject·=·await·createToolObject(⏎················jsonToolFileContent,⏎················repositoryUrl,⏎················repoDescription,⏎················isAsyncAPIrepo⏎··············

(prettier/prettier)


[error] 82-82: This line has a length of 123. Maximum allowed is 120.

(max-len)


[error] 84-84: Replace ············ with ··············

(prettier/prettier)


[error] 85-85: Replace await·Promise.all( with ··await·Promise.all(⏎················

(prettier/prettier)


[error] 86-86: Insert ····

(prettier/prettier)


[error] 87-87: Insert ····

(prettier/prettier)


[error] 88-88: Insert ····

(prettier/prettier)


[error] 89-89: Insert ····

(prettier/prettier)


[error] 90-90: Insert ····

(prettier/prettier)


[error] 91-91: Insert ····

(prettier/prettier)


[error] 92-92: Replace ············}) with ················})⏎··············

(prettier/prettier)


[error] 93-93: Replace ·········· with ············

(prettier/prettier)


[error] 94-94: Insert ··

(prettier/prettier)


[error] 95-95: Replace ············ with ··············

(prettier/prettier)


[error] 96-96: Insert ··

(prettier/prettier)


[error] 97-97: Replace ············ with ··············

(prettier/prettier)


[error] 98-98: Insert ··

(prettier/prettier)


[error] 99-99: Insert ··

(prettier/prettier)


[error] 100-100: Insert ··

(prettier/prettier)


[error] 101-101: Replace ········console.error(err) with ··········console.error(err);

(prettier/prettier)


[error] 102-102: Insert ··

(prettier/prettier)


[error] 103-103: Insert ··

(prettier/prettier)


[error] 104-104: Replace ····})) with ······})⏎····);

(prettier/prettier)


[error] 107-107: Insert ;

(prettier/prettier)


[error] 111-111: Insert ;

(prettier/prettier)

tests/helper/toolsObjectData.js

[error] 31-31: Replace ·repoUrl:·repoUrl·||·https://github.com/asyncapi/${title.toLowerCase().replace(/\s+/g,·'-')}`,·...additionalLinks` with ⏎····repoUrl:·repoUrl·||·https://github.com/asyncapi/${title.toLowerCase().replace(/\s+/g,·'-')}`,⏎····...additionalLinks⏎·`

(prettier/prettier)


[error] 44-44: Delete ·

(prettier/prettier)


[error] 53-53: Insert ··

(prettier/prettier)


[error] 63-63: Delete ·

(prettier/prettier)


[error] 66-66: Replace · with

(prettier/prettier)

🔇 Additional comments (5)
tests/helper/toolsObjectData.js (3)

35-53: LGTM! Well-structured composition with createToolFileContent.

The function correctly extends the tool object with the isAsyncAPIOwner property while maintaining all other functionality.

🧰 Tools
🪛 eslint

[error] 44-44: Delete ·

(prettier/prettier)


[error] 53-53: Insert ··

(prettier/prettier)


55-61: LGTM! Clean implementation with good type handling.

The function elegantly handles both string and object inputs for tool data.


63-74: 🛠️ Refactor suggestion

Improve YAML string construction safety.

The YAML string construction needs proper escaping of special characters and clearer indentation.

Consider this improvement from the previous review:

+const escapeYAML = (str) => {
+  return str.replace(/['"\\]/g, '\\$&');
+};
+
 const createMalformedYAML = ({
   title = 'Malformed Tool',
   description = 'This tool has malformed YAML.',
   repoUrl = 'https://github.com/asyncapi/malformed-repo'
 } = {}) => `
-  title: ${title}
-  description: ${description}
-  links:
-    repoUrl: ${repoUrl}
-  filters:
-    categories:
-      - Category1
+title: ${escapeYAML(title)}
+description: ${escapeYAML(description)}
+links:
+  repoUrl: ${escapeYAML(repoUrl)}
+filters:
+  categories:
+    - Category1
 `;
🧰 Tools
🪛 eslint

[error] 63-63: Delete ·

(prettier/prettier)


[error] 66-66: Replace · with

(prettier/prettier)

scripts/tools/tools-object.js (2)

53-62: LGTM! Efficient object initialization.

The use of Object.fromEntries with map is a performant way to initialize the object, avoiding the O(n²) complexity of spread in reduce.


111-111: LGTM! Clean exports statement.

The module exports follow proper formatting with consistent spacing.

🧰 Tools
🪛 eslint

[error] 111-111: Insert ;

(prettier/prettier)

tests/helper/toolsObjectData.js Outdated Show resolved Hide resolved
scripts/tools/tools-object.js Show resolved Hide resolved
scripts/tools/tools-object.js Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
tests/helper/toolsObjectData.js (1)

58-64: Consider adding input validation.

While the function handles both string and object inputs, it could benefit from additional validation:

  • Validate that tool objects contain required properties
  • Check for null/undefined values in the input array
 const createMockData = (tools = []) => ({
-  items: tools.map((tool) =>
+  items: tools.map((tool, index) => {
+    if (!tool) {
+      throw new Error(`Tool at index ${index} is null or undefined`);
+    }
+    return (
     typeof tool === 'string'
       ? createToolRepositoryData({ name: `.asyncapi-tool-${tool}`, repoName: tool })
       : createToolRepositoryData(tool)
-  )
+    );
+  })
 });
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dd54d8c and 840d13a.

📒 Files selected for processing (1)
  • tests/helper/toolsObjectData.js (1 hunks)
🔇 Additional comments (4)
tests/helper/toolsObjectData.js (4)

20-36: LGTM! Well-structured with proper URL encoding.

The function is well-implemented with:

  • Proper URL encoding for special characters
  • Good use of default values
  • Clean spread operator usage for extensibility

38-56: LGTM! Clean composition with createToolFileContent.

The function demonstrates good practices:

  • Clean composition with createToolFileContent
  • Proper handling of the isAsyncAPIOwner flag
  • Effective parameter forwarding

66-77: Improve YAML string safety.

The YAML string construction needs proper escaping of special characters to prevent malformed YAML when the input contains quotes or special characters.

This issue was previously identified in an earlier review. Please refer to the previous suggestion about implementing the escapeYAML function and applying proper YAML string escaping.


79-79: LGTM! Clean exports.

The module exports all necessary functions with clean syntax.

tests/helper/toolsObjectData.js Show resolved Hide resolved
@akshatnema
Copy link
Member

@coderabbitai resolve

Copy link

coderabbitai bot commented Dec 2, 2024

✅ Actions performed

Comments resolved.

@akshatnema
Copy link
Member

@coderabbitai full review

Copy link

coderabbitai bot commented Dec 2, 2024

✅ Actions performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
scripts/tools/tools-object.js (1)

82-82: Refactor line to comply with maximum line length

Line 82 exceeds the maximum line length of 120 characters. Refactoring it improves readability and adheres to coding standards.

Apply this diff to format the line:

-const toolObject = await createToolObject(jsonToolFileContent, repositoryUrl, repoDescription, isAsyncAPIrepo);
+const toolObject = await createToolObject(
+  jsonToolFileContent,
+  repositoryUrl,
+  repoDescription,
+  isAsyncAPIrepo
+);
🧰 Tools
🪛 eslint

[error] 82-82: Replace ············const·toolObject·=·await·createToolObject(jsonToolFileContent,·repositoryUrl,·repoDescription,·isAsyncAPIrepo with ··············const·toolObject·=·await·createToolObject(⏎················jsonToolFileContent,⏎················repositoryUrl,⏎················repoDescription,⏎················isAsyncAPIrepo⏎··············

(prettier/prettier)


[error] 82-82: This line has a length of 123. Maximum allowed is 120.

(max-len)

tests/helper/toolsObjectData.js (1)

69-69: Adjust function parameter formatting for consistency

Move the closing brace } to a new line to improve readability and adhere to formatting standards.

Apply this diff:

-  repoUrl = 'https://github.com/asyncapi/malformed-repo' } = {}) => `
+  repoUrl = 'https://github.com/asyncapi/malformed-repo' 
+} = {}) => `
🧰 Tools
🪛 eslint

[error] 69-69: Replace · with

(prettier/prettier)

tests/tools/tools-object.test.js (2)

1-2: Reorder imports to comply with module import conventions

Third-party modules should be imported before local modules. Move the axios import above the local module import.

Apply this diff:

-const { convertTools, createToolObject } = require('../../scripts/tools/tools-object');
-const axios = require('axios');
+const axios = require('axios');
+const { convertTools, createToolObject } = require('../../scripts/tools/tools-object');
🧰 Tools
🪛 eslint

[error] 2-2: axios import should occur before import of ../../scripts/tools/tools-object

(import/order)


25-25: Wrap arrow function parameter in parentheses

For consistency and clarity, wrap the arrow function parameter name in parentheses.

Apply this diff:

-const mockData = createMockData(toolNames.map(name => ({ name: `.asyncapi-tool-${name}`, repoName: name })));
+const mockData = createMockData(toolNames.map((name) => ({ name: `.asyncapi-tool-${name}`, repoName: name })));
🧰 Tools
🪛 eslint

[error] 25-25: Replace name with (name)

(prettier/prettier)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9e3696c and 840d13a.

📒 Files selected for processing (3)
  • scripts/tools/tools-object.js (2 hunks)
  • tests/helper/toolsObjectData.js (1 hunks)
  • tests/tools/tools-object.test.js (1 hunks)
🧰 Additional context used
📓 Learnings (2)
scripts/tools/tools-object.js (1)
Learnt from: akshatnema
PR: asyncapi/website#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.
tests/helper/toolsObjectData.js (1)
Learnt from: akshatnema
PR: asyncapi/website#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`.
🪛 eslint
tests/tools/tools-object.test.js

[error] 2-2: axios import should occur before import of ../../scripts/tools/tools-object

(import/order)


[error] 14-14: Delete ,

(prettier/prettier)


[error] 25-25: Replace name with (name)

(prettier/prettier)


[error] 45-50: Replace ⏎······toolFile,⏎······expected.links.repoUrl,⏎······'Repository·Description',⏎······true⏎···· with toolFile,·expected.links.repoUrl,·'Repository·Description',·true

(prettier/prettier)


[error] 123-123: Insert ⏎······

(prettier/prettier)


[error] 124-124: Insert ··

(prettier/prettier)


[error] 125-125: Insert ··

(prettier/prettier)


[error] 126-126: Replace } with ··}⏎····

(prettier/prettier)


[error] 141-141: Delete ,

(prettier/prettier)


[error] 143-143: Delete ··

(prettier/prettier)


[error] 145-145: Insert ⏎······

(prettier/prettier)


[error] 146-146: Insert ··

(prettier/prettier)


[error] 147-147: Insert ··

(prettier/prettier)


[error] 148-148: Insert ··

(prettier/prettier)


[error] 149-149: Replace } with ··}⏎····

(prettier/prettier)


[error] 150-150: Delete ··

(prettier/prettier)


[error] 152-152: Delete ··

(prettier/prettier)


[error] 154-154: Delete ··

(prettier/prettier)


[error] 156-156: Delete ··

(prettier/prettier)


[error] 159-160: Delete

(prettier/prettier)

scripts/tools/tools-object.js

[error] 64-64: Insert ⏎······

(prettier/prettier)


[error] 65-65: Insert ··

(prettier/prettier)


[error] 66-66: Insert ··

(prettier/prettier)


[error] 67-67: Insert ··

(prettier/prettier)


[error] 68-68: Insert ··

(prettier/prettier)


[error] 70-70: Insert ··

(prettier/prettier)


[error] 72-72: Insert ··

(prettier/prettier)


[error] 72-72: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 73-73: Replace ··········const·jsonToolFileContent·=·await·convertToJson(toolFileContent) with ············const·jsonToolFileContent·=·await·convertToJson(toolFileContent);

(prettier/prettier)


[error] 75-75: Insert ··

(prettier/prettier)


[error] 75-75: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 76-76: Replace const·isValid·=·await·validate(jsonToolFileContent) with ··const·isValid·=·await·validate(jsonToolFileContent);

(prettier/prettier)


[error] 78-78: Insert ··

(prettier/prettier)


[error] 79-79: Insert ··

(prettier/prettier)


[error] 80-80: Insert ··

(prettier/prettier)


[error] 81-81: Insert ··

(prettier/prettier)


[error] 82-82: Replace ············const·toolObject·=·await·createToolObject(jsonToolFileContent,·repositoryUrl,·repoDescription,·isAsyncAPIrepo with ··············const·toolObject·=·await·createToolObject(⏎················jsonToolFileContent,⏎················repositoryUrl,⏎················repoDescription,⏎················isAsyncAPIrepo⏎··············

(prettier/prettier)


[error] 82-82: This line has a length of 123. Maximum allowed is 120.

(max-len)


[error] 84-84: Replace ············ with ··············

(prettier/prettier)


[error] 85-85: Replace await·Promise.all( with ··await·Promise.all(⏎················

(prettier/prettier)


[error] 86-86: Insert ····

(prettier/prettier)


[error] 87-87: Insert ····

(prettier/prettier)


[error] 88-88: Insert ····

(prettier/prettier)


[error] 89-89: Insert ····

(prettier/prettier)


[error] 90-90: Insert ····

(prettier/prettier)


[error] 91-91: Insert ····

(prettier/prettier)


[error] 92-92: Replace ············}) with ················})⏎··············

(prettier/prettier)


[error] 93-93: Replace ·········· with ············

(prettier/prettier)


[error] 94-94: Insert ··

(prettier/prettier)


[error] 95-95: Replace ············ with ··············

(prettier/prettier)


[error] 96-96: Insert ··

(prettier/prettier)


[error] 97-97: Replace ············ with ··············

(prettier/prettier)


[error] 98-98: Insert ··

(prettier/prettier)


[error] 99-99: Insert ··

(prettier/prettier)


[error] 100-100: Insert ··

(prettier/prettier)


[error] 101-101: Replace ········console.error(err) with ··········console.error(err);

(prettier/prettier)


[error] 102-102: Insert ··

(prettier/prettier)


[error] 103-103: Insert ··

(prettier/prettier)


[error] 104-104: Replace ····})) with ······})⏎····);

(prettier/prettier)


[error] 107-107: Insert ;

(prettier/prettier)


[error] 111-111: Insert ;

(prettier/prettier)

tests/helper/toolsObjectData.js

[error] 69-69: Replace · with

(prettier/prettier)

🔇 Additional comments (2)
scripts/tools/tools-object.js (2)

68-68: Remove unused downloadUrl variable

The variable downloadUrl is declared but never used. Please remove it to clean up the code.

🧰 Tools
🪛 eslint

[error] 68-68: Insert ··

(prettier/prettier)


93-98: ⚠️ Potential issue

Throw an error when validation fails to ensure proper error propagation

Currently, when validation fails in the convertTools function, the errors are logged but not thrown. To ensure proper error handling and prevent silent failures, an error should be thrown when validation fails.

Apply this diff to throw an error on validation failure:

} else {
-  console.error('Script is not failing, it is just dropping errors for further investigation');
-  console.error('Invalid .asyncapi-tool file.');
-  console.error(`Located in: ${tool.html_url}`);
-  console.error('Validation errors:', JSON.stringify(validate.errors, null, 2));
+  const validationError = {
+    message: 'Validation failed for tool',
+    location: tool.html_url,
+    errors: validate.errors
+  };
+  throw new Error(JSON.stringify(validationError));
}
🧰 Tools
🪛 eslint

[error] 93-93: Replace ·········· with ············

(prettier/prettier)


[error] 94-94: Insert ··

(prettier/prettier)


[error] 95-95: Replace ············ with ··············

(prettier/prettier)


[error] 96-96: Insert ··

(prettier/prettier)


[error] 97-97: Replace ············ with ··············

(prettier/prettier)


[error] 98-98: Insert ··

(prettier/prettier)

@vishvamsinh28
Copy link
Contributor Author

@akshatnema applied 3 nitpick comments from coderabbit

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
scripts/tools/tools-object.js (1)

105-107: Enhance error handling in catch block.

The catch block should provide more context about where the error occurred.

       } catch (err) {
-        console.error(err)
-        throw err;
+        const errorContext = {
+          phase: 'tool_processing',
+          error: err.message,
+          toolUrl: tool.html_url
+        };
+        console.error('Tool processing failed:', JSON.stringify(errorContext));
+        throw new Error(`Failed to process tool ${tool.name}: ${err.message}`);
       }
🧰 Tools
🪛 eslint

[error] 105-105: Insert ··

(prettier/prettier)


[error] 106-106: Replace console.error(err) with ··console.error(err);

(prettier/prettier)


[error] 107-107: Insert ··

(prettier/prettier)

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

122-131: Enhance network error test case.

The network error test case could be more specific about the error context.

   it('should throw an error if axios.get fails', async () => {
+    const toolName = '.asyncapi-tool-error';
     const mockData = createMockData([{
-      name: '.asyncapi-tool-error',
+      name: toolName,
       repoName: 'error-tool'
     }]);

     axios.get.mockRejectedValue(new Error('Network Error'));

-    await expect(convertTools(mockData)).rejects.toThrow('Network Error');
+    await expect(convertTools(mockData)).rejects.toThrow(`Failed to process tool ${toolName}`);
+    expect(console.error).toHaveBeenCalledWith(
+      expect.stringContaining(JSON.stringify({
+        phase: 'tool_processing',
+        error: 'Network Error'
+      }))
+    );
   });
🧰 Tools
🪛 eslint

[error] 123-123: Insert ⏎······

(prettier/prettier)


[error] 124-124: Insert ··

(prettier/prettier)


[error] 125-125: Insert ··

(prettier/prettier)


[error] 126-126: Replace } with ··}⏎····

(prettier/prettier)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 840d13a and 2b56cb3.

📒 Files selected for processing (2)
  • scripts/tools/tools-object.js (2 hunks)
  • tests/tools/tools-object.test.js (1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/tools/tools-object.js (1)
Learnt from: akshatnema
PR: asyncapi/website#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.
🪛 eslint
scripts/tools/tools-object.js

[error] 64-64: Insert ⏎······

(prettier/prettier)


[error] 65-65: Insert ··

(prettier/prettier)


[error] 66-66: Insert ··

(prettier/prettier)


[error] 67-67: Insert ··

(prettier/prettier)


[error] 68-68: Insert ··

(prettier/prettier)


[error] 70-70: Insert ··

(prettier/prettier)


[error] 72-72: Insert ··

(prettier/prettier)


[error] 72-72: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 73-73: Replace ··········const·jsonToolFileContent·=·await·convertToJson(toolFileContent) with ············const·jsonToolFileContent·=·await·convertToJson(toolFileContent);

(prettier/prettier)


[error] 75-75: Insert ··

(prettier/prettier)


[error] 75-75: Expected exception block, space or tab after '//' in comment.

(spaced-comment)


[error] 76-76: Replace const·isValid·=·await·validate(jsonToolFileContent) with ··const·isValid·=·await·validate(jsonToolFileContent);

(prettier/prettier)


[error] 78-78: Insert ··

(prettier/prettier)


[error] 79-79: Insert ··

(prettier/prettier)


[error] 80-80: Insert ··

(prettier/prettier)


[error] 81-81: Insert ··

(prettier/prettier)


[error] 82-82: Insert ··

(prettier/prettier)


[error] 83-83: Insert ··

(prettier/prettier)


[error] 84-84: Insert ··

(prettier/prettier)


[error] 85-85: Insert ··

(prettier/prettier)


[error] 86-86: Replace ·············· with ················

(prettier/prettier)


[error] 87-87: Insert ··

(prettier/prettier)


[error] 89-89: Insert ··

(prettier/prettier)


[error] 90-90: Replace ············await·Promise.all( with ··············await·Promise.all(⏎················

(prettier/prettier)


[error] 91-91: Insert ····

(prettier/prettier)


[error] 92-92: Replace ·············· with ··················

(prettier/prettier)


[error] 93-93: Insert ····

(prettier/prettier)


[error] 94-94: Insert ····

(prettier/prettier)


[error] 95-95: Replace ················ with ····················

(prettier/prettier)


[error] 96-96: Insert ····

(prettier/prettier)


[error] 97-97: Replace ············}) with ················})⏎··············

(prettier/prettier)


[error] 98-98: Insert ··

(prettier/prettier)


[error] 99-99: Insert ··

(prettier/prettier)


[error] 100-100: Insert ··

(prettier/prettier)


[error] 101-101: Insert ··

(prettier/prettier)


[error] 102-102: Insert ··

(prettier/prettier)


[error] 103-103: Insert ··

(prettier/prettier)


[error] 104-104: Insert ··

(prettier/prettier)


[error] 105-105: Insert ··

(prettier/prettier)


[error] 106-106: Replace console.error(err) with ··console.error(err);

(prettier/prettier)


[error] 107-107: Insert ··

(prettier/prettier)


[error] 108-108: Replace ······ with ········

(prettier/prettier)


[error] 109-109: Replace ····})) with ······})⏎····);

(prettier/prettier)


[error] 112-112: Insert ;

(prettier/prettier)


[error] 116-116: Insert ;

(prettier/prettier)

tests/tools/tools-object.test.js

[error] 14-14: Delete ,

(prettier/prettier)


[error] 45-50: Replace ⏎······toolFile,⏎······expected.links.repoUrl,⏎······'Repository·Description',⏎······true⏎···· with toolFile,·expected.links.repoUrl,·'Repository·Description',·true

(prettier/prettier)


[error] 123-123: Insert ⏎······

(prettier/prettier)


[error] 124-124: Insert ··

(prettier/prettier)


[error] 125-125: Insert ··

(prettier/prettier)


[error] 126-126: Replace } with ··}⏎····

(prettier/prettier)


[error] 141-141: Delete ,

(prettier/prettier)


[error] 143-143: Delete ··

(prettier/prettier)


[error] 145-145: Insert ⏎······

(prettier/prettier)


[error] 146-146: Insert ··

(prettier/prettier)


[error] 147-147: Insert ··

(prettier/prettier)


[error] 148-148: Insert ··

(prettier/prettier)


[error] 149-149: Replace } with ··}⏎····

(prettier/prettier)


[error] 150-150: Delete ··

(prettier/prettier)


[error] 152-152: Delete ··

(prettier/prettier)


[error] 154-154: Delete ··

(prettier/prettier)


[error] 156-156: Delete ··

(prettier/prettier)


[error] 159-160: Delete

(prettier/prettier)

🔇 Additional comments (3)
scripts/tools/tools-object.js (3)

27-28: LGTM! Clean and well-structured function signature.

The function is well-organized with proper default values and clear parameter naming.


98-103: ⚠️ Potential issue

Improve validation error handling.

Based on previous feedback, validation errors should be thrown instead of just logged. Also, the error message "Script is not failing" is confusing.

           } else {
-            console.error('Script is not failing, it is just dropping errors for further investigation');
-            console.error('Invalid .asyncapi-tool file.');
-            console.error(`Located in: ${tool.html_url}`);
-            console.error('Validation errors:', JSON.stringify(validate.errors, null, 2));
+            const validationError = {
+              message: 'Invalid .asyncapi-tool file',
+              location: tool.html_url,
+              errors: validate.errors
+            };
+            throw new Error(JSON.stringify(validationError));
           }
⛔ Skipped due to learnings
Learnt from: akshatnema
PR: asyncapi/website#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.
🧰 Tools
🪛 eslint

[error] 98-98: Insert ··

(prettier/prettier)


[error] 99-99: Insert ··

(prettier/prettier)


[error] 100-100: Insert ··

(prettier/prettier)


[error] 101-101: Insert ··

(prettier/prettier)


[error] 102-102: Insert ··

(prettier/prettier)


[error] 103-103: Insert ··

(prettier/prettier)


64-70: 🛠️ Refactor suggestion

Remove unused downloadUrl variable.

The downloadUrl variable is constructed but never used. The axios request should use this URL directly.

-          const referenceId = tool.url.split('=')[1];
-          const downloadUrl = `https://raw.githubusercontent.com/${tool.repository.full_name}/${referenceId}/${tool.path}`;
-
-          const { data: toolFileContent } = await axios.get(downloadUrl);
+          const referenceId = tool.url.split('=')[1];
+          const { data: toolFileContent } = await axios.get(
+            `https://raw.githubusercontent.com/${tool.repository.full_name}/${referenceId}/${tool.path}`
+          );

Likely invalid or redundant comment.

🧰 Tools
🪛 eslint

[error] 64-64: Insert ⏎······

(prettier/prettier)


[error] 65-65: Insert ··

(prettier/prettier)


[error] 66-66: Insert ··

(prettier/prettier)


[error] 67-67: Insert ··

(prettier/prettier)


[error] 68-68: Insert ··

(prettier/prettier)


[error] 70-70: Insert ··

(prettier/prettier)

Comment on lines +75 to +86
it('should log errors for invalid .asyncapi-tool file', async () => {
const invalidContent = createToolFileContent({
title: 'Invalid Tool',
additionalFilters: { invalidField: true }
});
const mockData = mockToolData(invalidContent);

await convertTools(mockData);

expect(console.error).toHaveBeenCalledWith(expect.stringContaining('Script is not failing'));
expect(console.error).toHaveBeenCalledWith(expect.stringContaining('Invalid .asyncapi-tool file'));
});
Copy link

@coderabbitai coderabbitai bot Dec 2, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update error assertions to match new error handling.

The error assertions need to be updated to match the new error handling implementation.

     await convertTools(mockData);

-    expect(console.error).toHaveBeenCalledWith(expect.stringContaining('Script is not failing'));
-    expect(console.error).toHaveBeenCalledWith(expect.stringContaining('Invalid .asyncapi-tool file'));
+    await expect(convertTools(mockData)).rejects.toThrow('Invalid .asyncapi-tool file');
+    expect(console.error).toHaveBeenCalledWith(
+      expect.stringContaining(JSON.stringify({
+        message: 'Invalid .asyncapi-tool file',
+        location: expect.any(String),
+        errors: expect.any(Array)
+      }))
+    );

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

Choose a reason for hiding this comment

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

@vishvamsinh28 Can you please apply this suggestion?

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author

@vishvamsinh28 vishvamsinh28 Dec 2, 2024

Choose a reason for hiding this comment

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

@akshatnema This will fail the test because the script logs errors in multiple lines, whereas the suggestion expects a single log entry containing all error details as a JSON string. I have tested it locally.

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot
Copy link
Contributor

Hello, @akshatnema! 👋🏼
This PR is not up to date with the base branch and can't be merged.
Please update your branch manually with the latest version of the base branch.
PRO-TIP: To request an update from the upstream branch, simply comment /u or /update and our bot will handle the update operation promptly.

       The only requirement for this to work is to enable [Allow edits from maintainers](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) option in your PR. Also the update will not work if your fork is located in an organization, not under your personal profile.
       Thanks 😄

@vishvamsinh28
Copy link
Contributor Author

@akshatnema can you re run the workflow?

@asyncapi-bot asyncapi-bot merged commit 9976010 into asyncapi:master Dec 7, 2024
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc This label should be used for issues or discussions related to ideas for Google Summer of Code ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants