chore: Split derived.test.js to separate files. #38162
Conversation
- Deleted the unreadable `derived.test.js` (4668 lines) file and split each suite to its file. - Moved all specs in widget folder to __tests__ folder This refactor aims to improve maintainability and ensure that the table widget's derived properties are thoroughly tested.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (7)
You can disable this status message by setting the WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (21)
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getEditableCellValidity.test.js (3)
4-1247: Consider refactoring repetitive test patterns.The test suite is comprehensive and well-organized, but there's significant code duplication in the test cases. Consider using test.each() for similar test cases with different inputs.
Example refactor for the regex validation tests:
- it("should return true for editable columns when regex is matching", () => { - expect( - getEditableCellValidity( - { - isAddRowInProgress: true, - editableCell: {}, - newRow: { - step: "#1", - }, - primaryColumns: { - step: { - isEditable: true, - columnType: "text", - alias: "step", - validation: { - regex: "^#1$", - }, - }, - }, - }, - null, - _, - ), - ).toEqual({ step: true }); + test.each([ + ["#1", "^#1$", true], + ["test", "^test$", true], + ["test", "^#1$", false], + ["#1", "^test$", false], + ])("validates regex %s against pattern %s should be %s", (value, pattern, expected) => { + expect( + getEditableCellValidity( + { + isAddRowInProgress: true, + editableCell: {}, + newRow: { step: value }, + primaryColumns: { + step: { + isEditable: true, + columnType: "text", + alias: "step", + validation: { regex: pattern }, + }, + }, + }, + null, + _, + ), + ).toEqual({ step: expected }); + });
4-6: Consider adding test coverage for edge cases.While the test coverage is good for happy paths and basic validation scenarios, consider adding tests for:
- Invalid regex patterns
- Non-string values for regex validation
- Boundary conditions for min/max validations
14-15: Remove unnecessary null parameters.The test cases pass
nullas the second parameter togetEditableCellValidity. If this parameter is not used, consider removing it from the function signature.Also applies to: 436-437, 1061-1062
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getPageOffset.test.js (2)
1-1: Consider using path aliases for importsReplace the relative import with a path alias for better maintainability.
-import derivedProperty from "../../derived"; +import derivedProperty from "@widgets/TableWidgetV2/widget/derived";
4-80: Optimize test setup to reduce duplicationThe destructuring of
getPageOffsetis repeated in each test case. Consider moving it to the describe block level.describe("getPageOffset -", () => { + const { getPageOffset } = derivedProperty; + it("should return 0 when pageNo is null", () => { - const { getPageOffset } = derivedProperty;app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getFilteredTableData.test.js (4)
209-211: Use consistent assertion methodsConsider using
toEqualinstead oftoStrictEqualfor comparing objects that may have undefined or additional properties.Also applies to: 227-229, 386-388, 473-475, 638-640, 803-805, 925-927, 1190-1192, 1322-1324, 1343-1345, 1453-1455
448-448: Declare constants for expected dataUsing
constinstead ofletfor variables that are not reassigned improves code readability.Apply this change:
-let result = getFilteredTableData(input, moment, _); +const result = getFilteredTableData(input, moment, _);Also applies to: 616-616, 780-780, 919-919, 1185-1185, 1320-1320, 1341-1341, 1448-1448
61-211: Split test cases for clarityThe test case starting at line 61 is lengthy and covers multiple scenarios. Consider splitting it into smaller, focused tests for better readability and maintainability.
478-641: Handle null and empty string values consistentlyEnsure that the sorting logic correctly handles
nulland empty string values, and that this behavior aligns with the application's requirements.Also applies to: 643-806
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getUpdatedRowIndices.test.js (2)
34-46: Fix incorrect test descriptionThe test description states "returns empty array" but the test actually expects
[1]as the result.- it("should test that it returns empty array when transientTableData has one value", () => { + it("should test that it returns array with single index when transientTableData has one value", () => {
48-63: Fix incorrect test descriptionSimilar to the previous test, the description doesn't match the expected behavior.
- it("should test that it returns empty array when transientTableData has two value", () => { + it("should test that it returns array with two indices when transientTableData has two values", () => {app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/tableHeaders.test.js (1)
69-110: Add test case for invalid index valuesWhile sorting by index is tested, there's no coverage for invalid index values (non-numeric, negative, etc.).
Consider adding:
it("should handle invalid index values", () => { expect( getTableHeaders({ primaryColumns: { "value 1": { id: "value 1", label: "value 1", isVisible: true, index: "invalid" }, "value 2": { id: "value 2", label: "value 2", isVisible: true, index: -1 } } }) ).toEqual(/* expected behavior */); });app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRows.test.js (2)
1-3: Consider removing unused moment importThe
momentimport appears to be unused in most test cases. If it's required by thegetSelectedRowsfunction internally, consider documenting this requirement.
9-17: Move repeated test data to fixture fileThe same test data structure is repeated across multiple tests. Consider moving it to the fixture file for better maintainability.
// In fixture.js export const sampleTableData = [ { id: 1234, name: "Jim Doe", extra: "", __originalIndex__: 0 }, { id: 234, name: "Jane Doe", extra: "Extra2", __originalIndex__: 2 }, { id: 123, name: "John Doe", extra: "Extra1", __originalIndex__: 1 }, ];Also applies to: 30-38, 57-63, 70-77
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getUpdatedRow.test.js (2)
7-7: Test description needs to be more specificThe description "should check that valid updated row index returns the valid value" is vague. Consider renaming to "should return correct row data when given a valid row index".
38-83: Add test case for out-of-bounds indexWhile invalid indices are well tested, there's no coverage for an index that exceeds the array length.
Add this test case:
it("should return empty values when index exceeds array length", () => { const { getUpdatedRow } = derivedProperty; const input = { updatedRowIndex: 999, processedTableData: [ { id: 1, name: "Lorem Ipsum", extra: "", __originalIndex__: 0 }, ], }; expect(getUpdatedRow(input, moment, _)).toStrictEqual({ id: "", name: "", extra: "", }); });app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getOrderedTableColumns.test.js (3)
6-6: Fix typo in test description"should test tht it returns" contains a typo.
- it("should test tht it returns the columns array from the primaryColumn", () => { + it("should test that it returns the columns array from the primaryColumn", () => {
9-37: Consider adding test for missing column caseThe tests don't cover the scenario where a column exists in columnOrder but not in primaryColumns.
Add this test case:
it("should handle columns that exist in columnOrder but not in primaryColumns", () => { const { getOrderedTableColumns } = derivedProperty; const input = { columnOrder: ["id", "name", "nonexistent"], primaryColumns: { id: { index: 0, id: "id" }, name: { index: 1, id: "name" }, }, }; const expected = [ { index: 0, id: "id", isAscOrder: undefined }, { index: 1, id: "name", isAscOrder: undefined }, ]; expect(getOrderedTableColumns(input, moment, _)).toStrictEqual(expected); });
75-140: Reduce code duplication in sort order testsThe test data and expectations for ascending and descending sort orders could be more DRY using a test.each pattern.
it.each([ ['asc', true], ['desc', false] ])('should set isAscOrder to %p when sort order is %p', (order, expected) => { const input = { columnOrder: ["name", "id"], primaryColumns: { id: { index: 0, id: "id" }, name: { index: 1, id: "name" }, }, sortOrder: { column: "name", order, }, }; expect(getOrderedTableColumns(input, moment, _).find(col => col.id === "name").isAscOrder) .toBe(expected); });app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/fixture.js (1)
93-94: Simplify the onClick handler using a switch statement.The current nested ternary operation could be harder to maintain. Consider using a switch statement for better readability.
- onClick: - "{{currentRow.step === '#1' ? showAlert('Done', 'success') : currentRow.step === '#2' ? navigateTo('https://docs.appsmith.com/core-concepts/connecting-to-data-sources/querying-a-database',undefined,'NEW_WINDOW') : navigateTo('https://docs.appsmith.com/core-concepts/displaying-data-read/display-data-tables',undefined,'NEW_WINDOW')}}", + onClick: `{{(() => { + switch(currentRow.step) { + case '#1': + return showAlert('Done', 'success'); + case '#2': + return navigateTo('https://docs.appsmith.com/core-concepts/connecting-to-data-sources/querying-a-database',undefined,'NEW_WINDOW'); + default: + return navigateTo('https://docs.appsmith.com/core-concepts/displaying-data-read/display-data-tables',undefined,'NEW_WINDOW'); + } + })()}}`,app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRow.test.js (1)
6-238: Consider adding test cases for additional edge scenarios.The test suite has good coverage but could benefit from additional edge cases:
- Empty
processedTableDataarray- Out of bounds indices (e.g., very large numbers)
- Negative indices other than -1
Example test case:
it("Single row selection, with empty processedTableData", () => { const { getSelectedRow } = derivedProperty; const input = { multiRowSelection: false, selectedRowIndices: [], selectedRowIndex: 0, processedTableData: [], }; expect(getSelectedRow(input, moment, _)).toStrictEqual({ id: "", name: "", extra: "", }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/client/src/widgets/TableWidgetV2/widget/__tests__/TableRendered.test.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/fixture.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getEditableCellValidity.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getFilteredTableData.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getOrderedTableColumns.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getPageOffset.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRow.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRows.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getTriggeredRow.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getUpdatedRow.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getUpdatedRowIndices.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getUpdatedRows.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/htmlColumns.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/tableHeaders.test.js(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/propertyUtils.test.ts(1 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/utilities.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/widgets/TableWidgetV2/widget/tests/propertyUtils.test.ts
- app/client/src/widgets/TableWidgetV2/widget/tests/utilities.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/htmlColumns.test.js
[error] 183-183: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 212-212: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 292-292: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 306-306: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 390-390: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 442-442: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 525-525: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 577-577: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (11)
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getEditableCellValidity.test.js (1)
1-3: LGTM! Clean imports section.
The imports are minimal and well-organized.
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/htmlColumns.test.js (1)
169-579: Well-structured test cases with comprehensive coverage
The test suite effectively covers:
- Basic HTML content handling
- Complex nested HTML structures
- Special characters and entities
- Edge cases with null/undefined values
🧰 Tools
🪛 Biome (1.9.4)
[error] 183-183: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 212-212: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 292-292: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 306-306: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 390-390: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 442-442: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 525-525: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 577-577: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getFilteredTableData.test.js (3)
391-476: Ensure test data reflects realistic scenarios
In the test case starting at line 391, verify that the edited values and sorting orders accurately simulate real-world use cases.
1195-1277: Validate key sanitization logic
In the test starting at line 1195, confirm that all special characters and non-standard keys are properly sanitized and that the resulting keys are valid for downstream use.
1080-1193: Confirm filtering after data modification
After editing values with applied filters or search keys, ensure that the getFilteredTableData function updates the results as expected.
Also applies to: 1348-1456
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getUpdatedRowIndices.test.js (1)
1-64: LGTM! Test coverage is comprehensive
The test suite effectively covers:
- Edge cases (empty, null, undefined)
- Single and multiple entry scenarios
- Input validation
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/tableHeaders.test.js (1)
1-111: LGTM! Well-structured test suite
The test suite has:
- Good edge case coverage
- Clear test descriptions
- Proper validation of sorting functionality
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRows.test.js (1)
1-99: LGTM! Comprehensive test coverage
The test suite effectively covers:
- Valid and invalid indices
- Single and multiple row selection
- Data transformation and filtering
app/client/src/widgets/TableWidgetV2/widget/__tests__/TableRendered.test.ts (1)
2-3: LGTM! Import paths updated correctly.
The import paths have been updated to reflect the new directory structure.
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getUpdatedRows.test.js (1)
6-243: LGTM! Well-structured and comprehensive test coverage.
The test suite provides excellent coverage with:
- Edge cases (empty, null, undefined)
- Invalid data scenarios
- Incremental complexity in valid data cases
- Integration with fixture data
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRow.test.js (1)
1-5: LGTM! Well-organized imports and test setup.
Good practice to separate test fixtures into a dedicated file.
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getPageOffset.test.js
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/htmlColumns.test.js
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/htmlColumns.test.js
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getFilteredTableData.test.js
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getTriggeredRow.test.js
Show resolved
Hide resolved
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRow.test.js
Outdated
Show resolved
Hide resolved
|
@rahulbarwal why aren't we running tests on these? |
…rmatting - Reformatted options in jest.config.js for better readability. - Enhanced testRegex to support more flexible test file matching patterns.
@jacquesikot The change has nothing that impact the product or cypress tests. I don't the need to run cypress on this one. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRow.test.js (4)
1-5: Remove unused moment importThe moment library is imported but not utilized meaningfully in the tests. Consider removing it to keep the imports clean.
-import moment from "moment";
7-7: Fix typo in test descriptionThere's a typo in the test description.
- it("Multple row selection, with selected rows", () => { + it("Multiple row selection, with selected rows", () => {
87-225: Reduce test data duplication using beforeEachConsider extracting the common test data structure into a beforeEach block to reduce duplication and improve maintainability.
describe("Invalid input scenarios", () => { let baseInput; beforeEach(() => { baseInput = { multiRowSelection: true, selectedRowIndex: -1, processedTableData: [ { id: 1234, name: "Jim Doe", extra: "", __originalIndex__: 0 }, { id: 234, name: "Jane Doe", extra: "Extra2", __originalIndex__: 2 }, { id: 123, name: "John Doe", extra: "Extra1", __originalIndex__: 1 }, ], }; }); it("should return empty row when selectedRowIndices is a string value", () => { expect(getSelectedRow({ ...baseInput, selectedRowIndices: ["test"] }, moment, _)) .toStrictEqual({ id: "", name: "", extra: "", }); }); // ... other test cases });
227-243: Document non-data columns criteriaAdd a comment explaining what constitutes a non-data column to improve test clarity and maintainability.
+ // Non-data columns are columns that are used for internal purposes + // and should not be included in the selected row data it("Single row selection should not have non data columns", () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/jest.config.js(2 hunks)app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRow.test.js(1 hunks)
🔇 Additional comments (4)
app/client/jest.config.js (3)
26-28: LGTM - Formatting changes only
The restructuring of the options object improves readability.
Also applies to: 32-32
74-74: Verify the entities path mapping
The addition of the entities path mapping looks good, but let's verify its usage.
✅ Verification successful
The entities path mapping is correctly used across the codebase
The search results show extensive usage of the entities/* import pattern across multiple files in the codebase, particularly in:
- Test files
- Widget components
- Workers
- Sagas
- Utils
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for imports using the entities path
# Expected: Should find imports using the entities/* pattern
rg -g '*.{ts,tsx,js,jsx}' "from ['|\"]entities/" -A 1
Length of output: 65964
38-39: Verify the test discovery pattern
The new regex pattern allows for test files both inside and outside the __tests__ directory, which aligns with the PR's objective of splitting derived.test.js. Let's verify this works as intended.
✅ Verification successful
Based on the output from the fd command, I can verify that the test pattern in jest.config.js is working correctly. The pattern successfully matches test files both inside and outside the __tests__ directory, which is exactly what we want. Let me generate the final response:
Test pattern correctly matches files in both locations
The regex pattern successfully discovers test files both inside and outside __tests__ directories, which is the intended behavior for splitting derived.test.js. This is verified by finding:
- Test files inside
__tests__: e.g.app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/ - Test files outside
__tests__: e.g.app/client/src/widgets/TableWidgetV2/widget/derived.test.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new test pattern matches both test locations
# Expected: Should find test files both inside and outside __tests__ directory
# Check test files in __tests__ directory
echo "Tests in __tests__ directory:"
fd -e test.ts -e test.tsx -e test.js -e test.jsx -p "__tests__/"
echo -e "\nTests outside __tests__ directory:"
fd -e test.ts -e test.tsx -e test.js -e test.jsx -E "__tests__/"
Length of output: 32862
app/client/src/widgets/TableWidgetV2/widget/__tests__/derived.test/getSelectedRow.test.js (1)
6-244: Verify test coverage for getSelectedRow function
The test suite appears comprehensive. Let's verify the coverage to ensure all code paths are tested.
rahulbarwal
added
the
Widgets & Accelerators Pod
Description
This PR started with this goal:
derived.test.js(4668 lines) file and split each suite to its file.derived.jsrelated specs in widget folder to tests folderLater we found that
testRegexinjest.configwill treat anything inside__tests__as runnable, so we modify this rule and are moving to a consistent naming for our unit tests(any file ending with.test.or.spec.This refactor aims to improve maintainability and ensure that the table widget's derived properties are thoroughly tested.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD b3168bb yet
Tue, 17 Dec 2024 10:20:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores