feat: add test for build docs script #3137
Conversation
vishvamsinh28
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
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 site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3137--asyncapi-website.netlify.app/ |
anshgoyalevil
added
the
gsoc
| try { | ||
| addDocButtons(docPosts, invalidTreePosts); | ||
| } catch (err) { | ||
| expect(err.message).toMatch(/An error occurred while adding doc buttons:/); |
There was a problem hiding this comment.
This expect statement is not being executed here. Try modiying the line /An error occurred while adding doc buttons:/ and you would observe it is always passing the test.
| it('should throw and catch an error if no valid specification version is found', () => { | ||
|
|
||
| try { | ||
| buildNavTree(missingSpecVersion); | ||
| } catch (err) { | ||
| expect(err.message).toContain('No valid specification version found'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
We don't really throw any such error inside the buildNavTree function. The control is never going inside catch block and so this test is passing, like the one is addDocButtons test.
Use GPT with care 😸
There was a problem hiding this comment.
Yea 😅 trying to complete the project on time 🫠
| const { buildNavTree } = require('../../scripts/build-docs'); | ||
| const { basicNavItems, sectionNavItems, orphanNavItems, missingSpecVersion, nullNavItems } = require('../fixtures/buildNavTreeData') | ||
|
|
||
| jest.mock('lodash/sortBy', () => jest.fn(arr => arr)); |
There was a problem hiding this comment.
I don't think there's a need to mock lodash sortBy. Let it run and expect the results to be sorted, as they should be with the real data.
| } catch (err) { | ||
| expect(err.message).toContain('Error in convertDocPosts:'); |
There was a problem hiding this comment.
this catch block is also not being executed
| } catch (err) { | ||
| expect(err.message).toContain('Error in convertDocPosts:'); |
| } catch (err) { | ||
| expect(err.message).toContain('Error in convertDocPosts:'); |
| } catch (err) { | ||
| expect(err.message).toContain('Error in convertDocPosts:'); |
There was a problem hiding this comment.
only this catch block is getting executed.
|
Will update the tests soon 👍 |
|
Make the functions error agnostic so you don't have to add a check for every possible error that could exist. The logic should be in such a way that the data type other than what is required should always throw an error, no matter it is a number, undefined, or anything else |
WalkthroughThe changes in this pull request enhance the Changes
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@anshgoyalevil test updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (20)
tests/fixtures/convertDocPostData.js (2)
13-24: LGTM: Comprehensive nested structureThe
nestedChildrenDocObjecteffectively represents a more complex document hierarchy with multiple nesting levels. This is excellent for testing scenarios with deeper structures.Consider adding a comment explaining the purpose of this complex structure and how it differs from the simpler
docObject. This could help other developers understand why both are necessary for testing.
1-31: Overall: Well-structured and comprehensive test fixturesThis file provides a robust set of test fixtures covering various document structure scenarios. The objects are consistently structured and well-exported, making them readily available for use in tests.
To further enhance the file:
- Consider adding a brief comment at the top of the file explaining its purpose and how these fixtures should be used.
- You might want to add JSDoc comments for each object to provide more detailed information about their structure and use cases.
These additions would improve the file's self-documentation and make it even easier for other developers to understand and use these fixtures.
tests/fixtures/addDocButtonsData.js (4)
1-3: Consider adding more test cases todocPosts.While the current structure is appropriate, having only one item in the
docPostsarray might limit its usefulness in testing various scenarios. Consider adding more mock documents with different titles, slugs, and content to ensure comprehensive testing.Would you like me to suggest additional test cases for
docPosts?
19-22: EnhancemockDocPostswith more diverse test cases.While the current
mockDocPostsprovides alternative titles and slugs, consider adding more diverse test cases to cover a wider range of scenarios. This could include:
- Posts with longer titles
- Posts with special characters in the slug
- Posts with nested paths in the slug
Here's an example of how you could expand
mockDocPosts:const mockDocPosts = [ { slug: '/docs', title: 'Welcome to Docs' }, { slug: '/docs/page1', title: 'Page 1' }, { slug: '/docs/getting-started/quick-start', title: 'Quick Start Guide' }, { slug: '/docs/advanced-topics/api-reference', title: 'API Reference Documentation' }, { slug: '/docs/troubleshooting/faq', title: 'Frequently Asked Questions (FAQ)' }, { slug: '/docs/special-characters/!@#$%^&*()', title: 'Special Characters in Slug' }, ];
39-39: ExpandinvalidTreePoststo cover more invalid scenarios.While the current
invalidTreePostsarray provides some invalid data for testing, it only includes string values. To ensure robust error handling, consider expanding this array to include a wider variety of invalid data types and structures.Here's a suggested expansion of
invalidTreePosts:const invalidTreePosts = [ 'tree1', 123, null, undefined, {}, [], { invalidKey: 'invalidValue' }, { item: 'Not an object' }, { item: {}, children: 'Not an object' }, ];This expanded array includes various data types and structures that should be considered invalid, allowing for more comprehensive testing of error handling in the code that uses this data.
1-41: Summary: Good foundation for test fixtures with room for improvementThis file provides a solid foundation for test fixtures related to documentation structure and content. However, there are several areas where it could be improved:
- Consistency: Ensure consistent structure across similar objects (e.g.,
treePostsandmockTreePosts).- Coverage: Expand test cases to cover a wider range of scenarios, especially for
docPostsandinvalidTreePosts.- Depth: Consider adding more complex nested structures to better test hierarchical document handling.
These improvements will enhance the robustness of tests using these fixtures and help catch edge cases in the code being tested.
Consider creating helper functions or a factory pattern to generate these test fixtures. This approach could make it easier to create varied test cases and maintain consistency across the fixtures. For example:
function createDocPost(title, slug, content) { return { title, slug, content }; } function createTreePost(title, isRootSection, slug, children = {}) { return { item: { title, isRootSection, slug }, children, }; } // Usage const docPosts = [ createDocPost('Welcome', '/docs', 'Welcome content'), createDocPost('Getting Started', '/docs/getting-started', 'Getting started content'), ]; const treePosts = { welcome: createTreePost('Welcome', true, '/docs', { introduction: createTreePost('Introduction', false, '/docs/introduction'), }), // ... more sections };This approach would make it easier to maintain consistency and generate a wide variety of test cases.
tests/build-docs/convertDocPosts.test.js (2)
38-48: Consider using expect().toThrow() for cleaner error testingWhile the current implementation correctly tests for the error scenario, consider using Jest's
expect().toThrow()method for a more concise and idiomatic approach. This would eliminate the need for the try-catch block and make the test more readable.Example:
it('should throw an error if docObject is undefined', () => { expect(() => convertDocPosts(undefined)).toThrow('Error in convertDocPosts:'); });This approach is more declarative and aligns with Jest's testing style.
1-62: Good test coverage with room for improvementOverall, this test file provides good coverage for the
convertDocPostsfunction, including happy paths, edge cases, and error scenarios. The use of fixture data is commendable. However, there are a few areas for potential improvement:
- Consider refactoring similar test cases (like the error tests) to reduce duplication.
- The PR objectives mention 80% test coverage. It might be beneficial to add more test cases to increase coverage further, especially for any untested edge cases or error scenarios.
- Consider adding tests for invalid input types (e.g., numbers, booleans) to make the function more robust.
- If possible, add tests that verify the entire structure of the returned objects, not just their titles.
These improvements could help achieve more comprehensive test coverage and make the tests more maintainable.
tests/build-docs/addDocButtons.test.js (1)
5-46: LGTM: Comprehensive test case for basic functionality.This test case thoroughly checks if the
addDocButtonsfunction correctly adds next and previous page information. It covers multiple scenarios and uses clear assertions.Consider adding a test for the last item in the result array to ensure it doesn't have a
nextPageproperty.tests/build-docs/buildNavTree.test.js (3)
17-55: LGTM: Comprehensive test for basic tree structure creation.This test case thoroughly checks the creation of the tree structure, including multiple levels and specific properties. It provides good coverage of the basic functionality.
Consider adding a test for the total number of root-level items to ensure no unexpected items are created:
expect(Object.keys(result).length).toBe(3); // Assuming 3 root-level items
89-99: Improve error message specificity for missing required fields.While this test case correctly verifies that an error is thrown for items with missing required fields, the error message is quite generic.
Consider updating the
buildNavTreefunction to provide more specific error messages for missing required fields. This would make debugging easier and improve the test's value. For example:expect(err.message).toContain('Missing required field(s)');Also, you might want to check for specific missing fields in the error message.
113-131: LGTM: Good test for sorting children based on weight.This test case effectively verifies the sorting of children within subsections based on weight. It covers multiple subsections and checks both the order of titles and the relative weights.
Consider adding a test for items with equal weights to ensure the sorting behavior is consistent in such cases. For example:
const equalWeightChildren = result['some-section'].children; expect(equalWeightChildren[0].weight).toBe(equalWeightChildren[1].weight); // Then check the order is consistent, perhaps based on title or another propertytests/fixtures/buildNavTreeData.js (7)
2-14: LGTM! Consider standardizing theisPrereleaseflag usage.The
basicNavItemsarray provides a well-structured representation of a basic navigation tree. It includes a mix of root sections, sections, and regular items, which is good for testing various scenarios.Consider standardizing the usage of the
isPrereleaseflag. It's present for v2.0 but missing for v3.0. If it's intentional, it might be worth adding a comment explaining why, or if not, add it to v3.0 for consistency:- { title: 'v3.0', weight: 2, isSection: false, rootSectionId: 'reference', sectionId: 'specification', slug: '/docs/reference/specification/v3.0' } + { title: 'v3.0', weight: 2, isSection: false, rootSectionId: 'reference', sectionId: 'specification', slug: '/docs/reference/specification/v3.0', isPrerelease: false }
16-19: LGTM! Consider adding a clarifying comment.The
sectionNavItemsarray provides a minimal structure with a root section and an item without asectionId. This is useful for testing specific scenarios in the navigation tree building process.Consider adding a comment to explain the purpose of the item without a
sectionId:sectionNavItems: [ { title: 'Root', weight: 0, isRootSection: true, isSection: true, rootSectionId: 'root', sectionWeight: 0, slug: '/docs' }, + // This item is intentionally missing a sectionId to test error handling or default behavior { title: 'Item without sectionId', weight: 1, isSection: false, rootSectionId: 'root', slug: '/docs/item' }, ],
21-23: LGTM! Consider adding a clarifying comment.The
orphanNavItemsarray effectively provides a test case for handling orphaned items in the navigation tree. The item intentionally references a non-existent parent, which is useful for testing error handling or special processing of such cases.Consider adding a comment to explicitly state the purpose of this test case:
orphanNavItems: [ + // This item intentionally references a non-existent parent to test handling of orphaned items { title: 'Orphaned Subsection', weight: 0, isSection: true, rootSectionId: 'root', sectionId: 'orphan', parent: 'non-existent-parent', slug: '/docs/orphaned' } ],
25-27: LGTM! Consider adding a clarifying comment.The
missingFieldsNavItemsarray provides a good test case for handling items with incomplete data. The item intentionally omits some fields that are present in other navigation items, which is useful for testing robustness and error handling in the navigation tree building process.Consider adding a comment to explicitly state which fields are intentionally omitted and why:
missingFieldsNavItems: [ + // This item intentionally omits isRootSection and sectionId fields to test handling of incomplete data { title: 'Incomplete Item', weight: 0, isSection: false, rootSectionId: 'incomplete', slug: '/docs/incomplete' }, ],
29-32: LGTM! Consider adding a clarifying comment.The
invalidParentNavItemsarray effectively provides test cases for handling both valid and invalid parent references in the navigation tree. It includes a valid root section and a child item that intentionally references a non-existent parent, which is useful for testing error handling or special processing of such cases.Consider adding comments to explicitly state the purpose of each item:
invalidParentNavItems: [ + // Valid root section { title: 'Valid Root', weight: 0, isRootSection: true, isSection: true, rootSectionId: 'valid-root', sectionWeight: 0, slug: '/docs/valid-root' }, + // Child item intentionally referencing a non-existent parent to test handling of invalid parent references { title: 'Child with invalid parent', weight: 1, isSection: true, rootSectionId: 'valid-root', sectionId: 'child-invalid', parent: 'non-existent-parent', slug: '/docs/valid-root/child-invalid' }, ],
34-44: LGTM! Consider clarifying the weight assignment strategy.The
multipleSubsectionsNavItemsarray provides a comprehensive test case for a complex navigation structure with multiple levels and subsections. This is excellent for testing the navigation tree building process in more realistic scenarios.The weight values seem to follow different patterns for different sections. For example, the API subsection uses small incremental weights (1, 2, 3), while the Specification subsection uses larger increments (10, 20, 30). If this is intentional, consider adding a comment to explain the reasoning:
multipleSubsectionsNavItems: [ + // Note: Weight values are intentionally varied to test different sorting scenarios { title: 'Reference', weight: 0, isRootSection: true, isSection: true, rootSectionId: 'reference', sectionWeight: 0, slug: '/docs/reference' }, // ... (rest of the items) ]This will help other developers understand that the varied weight values are not a mistake but a deliberate choice for testing purposes.
1-45: Overall, excellent test fixtures for navigation tree building!This file provides a comprehensive set of test fixtures for various scenarios in navigation tree building. The different arrays (
basicNavItems,sectionNavItems,orphanNavItems,missingFieldsNavItems,invalidParentNavItems, andmultipleSubsectionsNavItems) cover a wide range of cases, including:
- Basic navigation structures
- Items with missing fields
- Orphaned items
- Invalid parent references
- Complex multi-level structures
These fixtures will be valuable for thorough testing of the navigation tree building functionality.
Consider adding a brief comment at the top of the file explaining its purpose and the different scenarios covered by each array. This will help other developers quickly understand the intent and structure of these test fixtures.
scripts/build-docs.js (1)
163-164: Correct Typographical Error in CommentThere's a typographical error in the comment: "Segementation fault" should be "Segmentation fault." Also, consider clarifying the comment for better understanding.
Apply this diff to fix the typo and improve the comment:
- // additonal check for the first page of Docs so that it doesn't give any Segementation fault + // Additional check for the first page of Docs to prevent any segmentation faults
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- scripts/build-docs.js (1 hunks)
- tests/build-docs/addDocButtons.test.js (1 hunks)
- tests/build-docs/buildNavTree.test.js (1 hunks)
- tests/build-docs/convertDocPosts.test.js (1 hunks)
- tests/fixtures/addDocButtonsData.js (1 hunks)
- tests/fixtures/buildNavTreeData.js (1 hunks)
- tests/fixtures/convertDocPostData.js (1 hunks)
🔇 Additional comments (20)
tests/fixtures/convertDocPostData.js (4)
1-7: LGTM: Well-structured docObjectThe
docObjectis well-structured and clearly represents a hierarchical document with a root and two child items. The consistent use ofitemandchildrenproperties makes the structure easy to understand and work with.
9-9: LGTM: Useful empty object fixtureThe
emptyDocObjectis a simple but valuable test fixture for handling edge cases or scenarios with no data.
11-11: LGTM: Useful single post fixtureThe
singlePostDocObjectis a good representation of a standalone post or document. Its structure is consistent with the items indocObject, which is excellent for maintaining uniformity across different test scenarios.
26-31: LGTM: Clean and comprehensive exportsThe
module.exportsstatement correctly exports all four objects using the concise shorthand notation. This makes the fixtures easily accessible for import in test files.tests/fixtures/addDocButtonsData.js (1)
41-41: LGTM: Module exports are correct and comprehensive.The module exports statement correctly includes all defined constants with clear and consistent naming. This allows for easy import and use of these fixtures in test files.
tests/build-docs/convertDocPosts.test.js (5)
1-7: LGTM: Imports are well-structuredThe import statements are clear and follow good practices. Importing the function under test and using fixture data for tests enhances maintainability and readability.
10-16: LGTM: Well-structured basic functionality testThis test case effectively covers the primary functionality of
convertDocPosts. It checks both the structure (array length) and content (titles) of the result, providing good coverage for the happy path.
18-21: LGTM: Good edge case coverageThis test case effectively handles the edge case of an empty doc object input. It clearly defines and tests the expected behavior, which is crucial for robust error handling.
23-27: LGTM: Comprehensive edge case handlingThis test case effectively covers the scenario of a doc object with no children, which is another important edge case. The expected behavior is well-defined and properly tested.
29-36: LGTM: Thorough testing of complex scenariosThis test case effectively handles a more complex scenario with nested children. It provides comprehensive coverage by checking both the structure (array length) and content (titles) of the result, ensuring the function correctly processes nested hierarchies.
tests/build-docs/addDocButtons.test.js (3)
1-2: LGTM: Imports are correctly set up.The necessary function and test data are properly imported for the test suite.
48-54: LGTM: Well-defined test for edge case.This test case effectively verifies the behavior of
addDocButtonswhen the next item is a root element. It uses mock data and checks both the title and href of the nextPage property.
1-91: Overall, well-structured and comprehensive test suite with room for minor improvements.This test file for the
addDocButtonsfunction covers a wide range of scenarios, including normal functionality, edge cases, and error handling. The test cases are well-organized and clearly written.Main points for improvement:
- Enhance error checking in the last three test cases by moving the error message assertion outside the try-catch block.
- Consider refactoring the similar error-checking test cases to reduce code duplication.
- Add a test for the last item in the result array to ensure it doesn't have a
nextPageproperty.These improvements will further increase the reliability and maintainability of the test suite.
tests/build-docs/buildNavTree.test.js (4)
1-10: LGTM: Imports and test data setup look good.The imports are correctly structured, and using separate test data fixtures is a good practice for maintainability.
57-75: LGTM: Good test for handling items without sectionId.This test case effectively verifies the correct handling of items without a sectionId, including the creation of a root item and proper placement of the child item.
1-133: Overall, a well-structured and comprehensive test suite.This test suite provides good coverage of the
buildNavTreefunction's functionality, including basic tree structure creation, handling of items without sectionId, error cases, and sorting based on weight.Some suggestions for improvement:
- Verify and align error handling tests with the actual function behavior.
- Consider consolidating similar error handling tests.
- Add tests for edge cases like items with equal weights.
- Improve error message specificity where applicable.
These changes will further enhance the robustness and maintainability of the test suite.
77-87:⚠️ Potential issueVerify error handling in the buildNavTree function.
This test case expects an error to be thrown when a parent section is missing. However, based on the past review comment, it appears that the
buildNavTreefunction may not actually throw this error.Please verify the error handling in the
buildNavTreefunction. If the function doesn't throw an error for missing parent sections, consider either:
- Updating the function to throw the expected error, or
- Modifying this test to match the actual behavior of the function.
To help verify this, you can run the following script:
scripts/build-docs.js (3)
3-75: FunctionbuildNavTreeReviewedThe
buildNavTreefunction has been updated with atry-catchblock for improved error handling. The usage of optional chaining (?.) enhances the safety of property access. The overall logic appears sound and correctly builds the navigation tree.
81-93: FunctionconvertDocPostsLogic VerifiedThe recursive function
convertDocPostscorrectly traverses the document tree and aggregates the posts. With the addition of thetry-catchblock, error handling is enhanced.
100-180: FunctionaddDocButtonsLogic ReviewedThe
addDocButtonsfunction effectively addsnextPageandprevPagedetails to each document post. Aside from the issues noted regarding error handling and index boundary checks, the function logic is solid.
| const treePosts = { | ||
| welcome: { | ||
| item: { title: 'Welcome', isRootSection: true, slug: '/docs' }, | ||
| children: {}, | ||
| }, | ||
| section1: { | ||
| item: { title: 'Section 1', isRootSection: true }, | ||
| children: { | ||
| page1: { item: { title: 'Page 1', slug: '/docs/section1/page1' } }, | ||
| page2: { item: { title: 'Page 2', slug: '/docs/section1/page2' } }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Ensure consistency in treePosts structure.
There's an inconsistency in the structure between 'welcome' and 'section1':
- 'welcome' has a
slugproperty, while 'section1' doesn't. - 'welcome' has an empty
childrenobject, while 'section1' has child pages.
To maintain consistency and improve testability, consider:
- Adding a
slugto 'section1'. - Adding child pages to 'welcome' or removing its
childrenproperty if it's meant to be a leaf node.
Here's a suggested structure for consistency:
const treePosts = {
welcome: {
item: { title: 'Welcome', isRootSection: true, slug: '/docs' },
children: {
introduction: { item: { title: 'Introduction', slug: '/docs/introduction' } },
},
},
section1: {
item: { title: 'Section 1', isRootSection: true, slug: '/docs/section1' },
children: {
page1: { item: { title: 'Page 1', slug: '/docs/section1/page1' } },
page2: { item: { title: 'Page 2', slug: '/docs/section1/page2' } },
},
},
};| const mockTreePosts = { | ||
| root1: { | ||
| item: { title: 'Root 1', isRootSection: true }, | ||
| children: { | ||
| child1: { item: { title: 'Child 1', slug: '/docs/root1/child1' } }, | ||
| }, | ||
| }, | ||
| root2: { | ||
| item: { title: 'Root 2', isRootElement: true }, | ||
| children: { | ||
| child2: { item: { title: 'Child 2', slug: '/docs/root2/child2' } }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Address inconsistencies in mockTreePosts structure.
There are a few inconsistencies in the mockTreePosts structure:
- The
isRootSectionproperty is namedisRootElementin 'root2'. - Root sections don't have
slugproperties, unlike intreePosts. - There's no variation in the depth of the tree structure.
Consider the following improvements:
- Standardize the property name to
isRootSection. - Add
slugproperties to root sections for consistency withtreePosts. - Include a more complex tree structure with varying depths.
Here's a suggested structure:
const mockTreePosts = {
root1: {
item: { title: 'Root 1', isRootSection: true, slug: '/docs/root1' },
children: {
child1: {
item: { title: 'Child 1', slug: '/docs/root1/child1' },
children: {
grandchild1: { item: { title: 'Grandchild 1', slug: '/docs/root1/child1/grandchild1' } },
},
},
},
},
root2: {
item: { title: 'Root 2', isRootSection: true, slug: '/docs/root2' },
children: {
child2: { item: { title: 'Child 2', slug: '/docs/root2/child2' } },
},
},
};| it('should throw an error if docObject is null', () => { | ||
| let error; | ||
|
|
||
| try { | ||
| convertDocPosts(null); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(err.message).toContain('Error in convertDocPosts:'); | ||
| } | ||
| expect(error).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring similar error test cases
This test case is very similar to the previous one, testing for null instead of undefined. To improve maintainability and reduce code duplication, consider refactoring these similar test cases into a single parameterized test.
Example using Jest's test.each:
test.each([
['undefined', undefined],
['null', null]
])('should throw an error if docObject is %s', (_, input) => {
expect(() => convertDocPosts(input)).toThrow('Error in convertDocPosts:');
});This approach reduces duplication and makes it easier to add more invalid input types in the future.
| it('should handle invalid data structure in treePosts', () => { | ||
| let error; | ||
|
|
||
| try { | ||
| addDocButtons(docPosts, invalidTreePosts); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(err.message).toContain("An error occurred while adding doc buttons:"); | ||
| } | ||
| expect(error).toBeDefined() | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same improvement and consider refactoring.
This test case for handling invalid data structure in treePosts has the same structure as the previous two.
- Apply the same improvement as suggested for the previous test cases:
try {
addDocButtons(docPosts, invalidTreePosts);
} catch (err) {
error = err;
- expect(err.message).toContain("An error occurred while adding doc buttons:");
}
expect(error).toBeDefined()
+ expect(error.message).toContain("An error occurred while adding doc buttons:");- Consider refactoring these similar test cases to reduce code duplication. You could create a helper function:
function testErrorThrown(testFn, errorMessage) {
let error;
try {
testFn();
} catch (err) {
error = err;
}
expect(error).toBeDefined();
expect(error.message).toContain(errorMessage);
}
// Usage:
it('should throw an error if treePosts is missing', () => {
testErrorThrown(
() => addDocButtons(docPosts, undefined),
"An error occurred while adding doc buttons:"
);
});
// Repeat for other similar test casesThis refactoring will make the tests more concise and easier to maintain.
| it('should throw an error if docPosts is missing', () => { | ||
| let error; | ||
|
|
||
| try { | ||
| addDocButtons(undefined, treePosts); | ||
| } catch (err) { | ||
| error = err | ||
| expect(err.message).toContain("An error occurred while adding doc buttons:"); | ||
| } | ||
| expect(error).toBeDefined() | ||
| }); |
There was a problem hiding this comment.
Apply the same improvement to this test case.
This test case for checking errors when docPosts is missing has the same structure as the previous one.
Apply the same improvement as suggested for the previous test case:
try {
addDocButtons(undefined, treePosts);
} catch (err) {
error = err
- expect(err.message).toContain("An error occurred while adding doc buttons:");
}
expect(error).toBeDefined()
+ expect(error.message).toContain("An error occurred while adding doc buttons:");This ensures that both the error existence and its message are always checked.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should throw an error if docPosts is missing', () => { | |
| let error; | |
| try { | |
| addDocButtons(undefined, treePosts); | |
| } catch (err) { | |
| error = err | |
| expect(err.message).toContain("An error occurred while adding doc buttons:"); | |
| } | |
| expect(error).toBeDefined() | |
| }); | |
| it('should throw an error if docPosts is missing', () => { | |
| let error; | |
| try { | |
| addDocButtons(undefined, treePosts); | |
| } catch (err) { | |
| error = err | |
| } | |
| expect(error).toBeDefined() | |
| expect(error.message).toContain("An error occurred while adding doc buttons:"); | |
| }); |
| it('should throw an error when parent references are invalid', () => { | ||
| let error; | ||
|
|
||
| try { | ||
| buildNavTree(invalidParentNavItems); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(err.message).toContain('Parent section non-existent-parent not found for item Child with invalid parent'); | ||
| } | ||
| expect(error).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating similar error handling tests.
This test case is very similar to the third test case ("should throw and catch an error if a parent section is missing"). Both are checking for errors related to invalid or missing parent sections.
Consider consolidating these two test cases into a single, parameterized test case. This would reduce duplication and make it easier to maintain these tests. For example:
it.each([
['missing parent', orphanNavItems, 'Parent section non-existent-parent not found for item Orphaned Subsection'],
['invalid parent', invalidParentNavItems, 'Parent section non-existent-parent not found for item Child with invalid parent']
])('should throw an error when %s', (_, navItems, expectedErrorMessage) => {
expect(() => buildNavTree(navItems)).toThrow(expectedErrorMessage);
});| throw new Error('Error in convertDocPosts:', err); | ||
| } |
There was a problem hiding this comment.
Include Original Error Message in 'convertDocPosts'
The error handling in the catch block does not correctly include the original error message. The Error constructor accepts a single message string, so the err parameter is ignored. To include the original error message, interpolate err.message into the new error message.
Apply this diff to fix the error handling:
} catch (err) {
- throw new Error('Error in convertDocPosts:', err);
+ throw new Error(`Error in convertDocPosts: ${err.message}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new Error('Error in convertDocPosts:', err); | |
| } | |
| throw new Error(`Error in convertDocPosts: ${err.message}`); | |
| } |
| throw new Error("An error occurred while adding doc buttons:", err); | ||
| } |
There was a problem hiding this comment.
Include Original Error Message in 'addDocButtons'
Similarly, the error handling in the addDocButtons function does not include the original error message. The additional err parameter is ignored by the Error constructor.
Apply this diff to correct the error handling:
} catch (err) {
- throw new Error("An error occurred while adding doc buttons:", err);
+ throw new Error(`An error occurred while adding doc buttons: ${err.message}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new Error("An error occurred while adding doc buttons:", err); | |
| } | |
| throw new Error(`An error occurred while adding doc buttons: ${err.message}`); | |
| } |
| if (!structuredPosts[index + 1].isRootElement && !structuredPosts[index + 1].isSection) { | ||
| nextPage = { | ||
| title: structuredPosts[index + 1].title, | ||
| href: structuredPosts[index + 1].slug | ||
| } | ||
| } else { | ||
| nextPage = { | ||
| title: `${structuredPosts[index + 1].title} - ${structuredPosts[index + 2].title}`, | ||
| href: structuredPosts[index + 2].slug | ||
| } |
There was a problem hiding this comment.
Prevent Potential Index Out-of-Bounds Errors in Next Page Logic
When accessing structuredPosts[index + 2], there's a risk of an index out-of-bounds error if index + 2 exceeds the array length. Ensure that index + 2 < countDocPages before accessing to prevent runtime errors.
Apply this diff to add the necessary boundary check:
if (!structuredPosts[index + 1].isRootElement && !structuredPosts[index + 1].isSection) {
nextPage = {
title: structuredPosts[index + 1].title,
href: structuredPosts[index + 1].slug
}
- } else {
+ } else if (index + 2 < countDocPages) {
nextPage = {
title: `${structuredPosts[index + 1].title} - ${structuredPosts[index + 2].title}`,
href: structuredPosts[index + 2].slug
}
+ } else {
+ // No next page available
+ nextPage = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!structuredPosts[index + 1].isRootElement && !structuredPosts[index + 1].isSection) { | |
| nextPage = { | |
| title: structuredPosts[index + 1].title, | |
| href: structuredPosts[index + 1].slug | |
| } | |
| } else { | |
| nextPage = { | |
| title: `${structuredPosts[index + 1].title} - ${structuredPosts[index + 2].title}`, | |
| href: structuredPosts[index + 2].slug | |
| } | |
| if (!structuredPosts[index + 1].isRootElement && !structuredPosts[index + 1].isSection) { | |
| nextPage = { | |
| title: structuredPosts[index + 1].title, | |
| href: structuredPosts[index + 1].slug | |
| } | |
| } else if (index + 2 < countDocPages) { | |
| nextPage = { | |
| title: `${structuredPosts[index + 1].title} - ${structuredPosts[index + 2].title}`, | |
| href: structuredPosts[index + 2].slug | |
| } | |
| } else { | |
| // No next page available | |
| nextPage = null; | |
| } |
| if (index - 2 >= 0) { | ||
| prevPage = { | ||
| title: `${structuredPosts[index - 1]?.isRootSection ? rootSections[rootSections.length - 2] : rootSections[rootSections.length - 1]} - ${structuredPosts[index - 2].title}`, | ||
| href: structuredPosts[index - 2].slug | ||
| }; | ||
| docPost = { ...docPost, prevPage }; |
There was a problem hiding this comment.
Prevent Potential Index Out-of-Bounds Errors in Previous Page Logic
Similarly, when accessing structuredPosts[index - 2], ensure that index - 2 >= 0 to avoid index out-of-bounds errors.
Apply this diff to adjust the condition:
if (index - 2 >= 0) {
prevPage = {
title: `${structuredPosts[index - 1]?.isRootSection ? rootSections[rootSections.length - 2] : rootSections[rootSections.length - 1]} - ${structuredPosts[index - 2].title}`,
href: structuredPosts[index - 2].slug
};
docPost = { ...docPost, prevPage };
+ } else {
+ // No previous page available
+ prevPage = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (index - 2 >= 0) { | |
| prevPage = { | |
| title: `${structuredPosts[index - 1]?.isRootSection ? rootSections[rootSections.length - 2] : rootSections[rootSections.length - 1]} - ${structuredPosts[index - 2].title}`, | |
| href: structuredPosts[index - 2].slug | |
| }; | |
| docPost = { ...docPost, prevPage }; | |
| if (index - 2 >= 0) { | |
| prevPage = { | |
| title: `${structuredPosts[index - 1]?.isRootSection ? rootSections[rootSections.length - 2] : rootSections[rootSections.length - 1]} - ${structuredPosts[index - 2].title}`, | |
| href: structuredPosts[index - 2].slug | |
| }; | |
| docPost = { ...docPost, prevPage }; | |
| } else { | |
| // No previous page available | |
| prevPage = null; | |
| } |
|
/rtm |
Co-authored-by: Ansh Goyal <[email protected]>
This PR adds test for build-docs.js script.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
addDocButtons,buildNavTree, andconvertDocPostsfunctions, covering normal functionality and edge cases.Documentation