test: add comprehensive unit tests for utility functions#41661
Open
xingzihai wants to merge 3 commits intoappsmithorg:releasefrom
Open
test: add comprehensive unit tests for utility functions#41661xingzihai wants to merge 3 commits intoappsmithorg:releasefrom
xingzihai wants to merge 3 commits intoappsmithorg:releasefrom
Conversation
- Add table of contents for easy navigation - Add project architecture overview with tech stack - Add quick start checklist for new contributors - Add finding issues section with categorized links - Add development setup quick guide - Add pull request process guidelines - Add community support channels - Improve overall structure and organization
- Add TypeHelpers.test.ts: tests for getType() and isURL() functions - Add URLUtils.test.ts: tests for URL manipulation utilities - Add dayJsUtils.test.ts: tests for date/time formatting functions - Add treeUtils.test.ts: tests for tree traversal and manipulation - Add getIsSafeURL.test.ts: tests for URL safety validation - Extend formhelpers.test.ts: comprehensive tests for email/password validation These tests improve coverage for core utility functions used throughout the application, including type checking, URL validation, date formatting, and form validation helpers.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a comprehensive CONTRIBUTING guide and introduces multiple new Jest test suites for client utils: type helpers, URL utilities, Day.js helpers, form helpers, tree utilities, and URL safety validation. All code changes are tests or documentation only. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
app/client/src/utils/formhelpers.test.ts (3)
77-80: Duplicate boundary tests.Lines 77-80 repeat the exact same boundary assertions from lines 54 and 56. Consider removing this redundant test block.
♻️ Proposed removal
- it("should return true for edge case lengths", () => { - expect(isStrongPassword("12345678")).toBe(true); // min length - expect(isStrongPassword("a".repeat(48))).toBe(true); // max length - }); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/utils/formhelpers.test.ts` around lines 77 - 80, The test file contains duplicated boundary assertions for isStrongPassword (the "should return true for edge case lengths" block repeating min/max length checks already asserted earlier); remove the redundant test block so each boundary case is asserted only once—locate the duplicate test by the description text or the calls to isStrongPassword("12345678") and isStrongPassword("a".repeat(48)) and delete that entire it(...) block.
226-232: Inconsistent import style forhashPassword.All other functions are imported at the top level (lines 1-9), but
hashPassworduses a dynamicrequire(). Add it to the top-level import for consistency.♻️ Proposed fix
Update the import statement:
import { isEmail, isEmptyString, isStrongPassword, noSpaces, isRelevantEmail, PASSWORD_MIN_LENGTH, PASSWORD_MAX_LENGTH, + hashPassword, } from "./formhelpers";Then simplify the test:
describe("hashPassword", () => { it("should return the password as-is", () => { - const { hashPassword } = require("./formhelpers"); expect(hashPassword("mypassword")).toBe("mypassword"); expect(hashPassword("")).toBe(""); expect(hashPassword("complexPassword123!@#")).toBe("complexPassword123!@#"); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/utils/formhelpers.test.ts` around lines 226 - 232, The test uses a dynamic require for hashPassword; instead, add hashPassword to the existing top-level import list (where other helpers are imported) and remove the inline require in the "hashPassword" test; then update the test body to call the top-level imported hashPassword directly (e.g., expect(hashPassword("...")).toBe(...)) so import style matches the other helpers and the test is simplified.
88-114: RenamenoSpacesfor clarity—function checks if value is empty or whitespace-only, not if it contains no space characters.The function at
app/client/src/utils/formhelpers.ts:21implementsreturn !value || value.trim().length === 0, which returnstruefor empty/whitespace-only strings. This behavior doesn't match the namenoSpaces, which implies "contains no space characters." Consider renaming toisEmptyOrWhitespaceOnlyor similar to align the name with actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/src/utils/formhelpers.test.ts` around lines 88 - 114, Rename the function noSpaces to a clearer name like isEmptyOrWhitespaceOnly (and update its export) because its implementation returns true for empty or whitespace-only strings rather than "no space characters"; update all references/imports (including the test file where noSpaces is used—e.g., the describe block in formhelpers.test.ts) to use the new name and adjust any JSDoc or comments to reflect the new semantic; ensure the function signature and return behavior remain unchanged so tests still assert the same boolean outcomes.CONTRIBUTING.md (2)
176-176: Minor: Consider hyphenating "Full Stack."For grammatical correctness, "Full-Stack Setup" (with hyphen) is preferred when used as a compound adjective.
Based on static analysis hints from LanguageTool.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 176, Change the heading text "Full Stack Setup" to the hyphenated form "Full-Stack Setup" in CONTRIBUTING.md; locate the header line containing the exact phrase "Full Stack Setup" and update it so the compound adjective uses a hyphen ("Full-Stack Setup") to match grammatical conventions and static analysis suggestions.
29-42: Consider adding language identifier to the fenced code block.The directory tree structure should specify a language identifier (e.g.,
textorbash) to satisfy markdown linters.📝 Proposed fix
-``` +```text appsmith/ ├── app/Based on static analysis hints from markdownlint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 29 - 42, Update the fenced code block showing the directory tree in CONTRIBUTING.md by adding a language identifier (e.g., change ``` to ```text) for the block that starts with "appsmith/"; this satisfies markdownlint rules for the tree snippet and keeps the visual formatting intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/src/utils/dayJsUtils.test.ts`:
- Around line 59-96: Tests in the getReadableDateInFormat suite use UTC-string
Date("...Z") fixtures which make assertions timezone-dependent; replace those
UTC-string constructors with local Date(...) constructors (e.g., new Date(2024,
2, 15, 12, 0, 0)) in each test case inside the "getReadableDateInFormat"
describe block so outputs (including the "should handle day names" / weekday
assertion) are consistent across timezones and still exercise
getReadableDateInFormat.
- Around line 3-214: Tests rely on English human-readable phrases but don't pin
dayjs locale, so add locale save/restore in this test suite: in the top-level
describe("dayJsUtils", ...) add a beforeAll that saves the current locale (via
dayjs.locale()) and then sets dayjs.locale("en"), and add an afterAll that
restores the saved locale; this ensures functions like getHumanizedTime,
getReadableDateInFormat and assertions in the "Integration tests" remain
deterministic while keeping references to getHumanizedTime,
getReadableDateInFormat, and dayjs unchanged.
In `@app/client/src/utils/TypeHelpers.test.ts`:
- Around line 195-200: The test description is contradictory: the "it" title
says URLs without protocol should be false but the assertions check for true;
update the test title to match the behavior (e.g., change the it(...)
description to "should return true for URLs without protocol") and optionally
adjust the inline comment to note that isURL accepts domain-only strings; the
change should be made around the test for isURL in TypeHelpers.test.ts where the
assertions expect true.
In `@app/client/src/utils/URLUtils.test.ts`:
- Around line 135-138: The test title is incorrect: it says "should handle array
values..." but passes a CSV string; update the test to keep behavior or change
input. Easiest fix: rename the test to "should handle CSV string by encoding
commas" (in the test that calls convertObjectToQueryParams({ items: "a,b,c" })
and expects "?items=a%2Cb%2Cc"). Alternatively, if you meant to test array
serialization, change the input to an actual array (e.g., { items: ["a","b","c"]
}) and adjust the expected value to match convertObjectToQueryParams' array
serialization behavior.
---
Nitpick comments:
In `@app/client/src/utils/formhelpers.test.ts`:
- Around line 77-80: The test file contains duplicated boundary assertions for
isStrongPassword (the "should return true for edge case lengths" block repeating
min/max length checks already asserted earlier); remove the redundant test block
so each boundary case is asserted only once—locate the duplicate test by the
description text or the calls to isStrongPassword("12345678") and
isStrongPassword("a".repeat(48)) and delete that entire it(...) block.
- Around line 226-232: The test uses a dynamic require for hashPassword;
instead, add hashPassword to the existing top-level import list (where other
helpers are imported) and remove the inline require in the "hashPassword" test;
then update the test body to call the top-level imported hashPassword directly
(e.g., expect(hashPassword("...")).toBe(...)) so import style matches the other
helpers and the test is simplified.
- Around line 88-114: Rename the function noSpaces to a clearer name like
isEmptyOrWhitespaceOnly (and update its export) because its implementation
returns true for empty or whitespace-only strings rather than "no space
characters"; update all references/imports (including the test file where
noSpaces is used—e.g., the describe block in formhelpers.test.ts) to use the new
name and adjust any JSDoc or comments to reflect the new semantic; ensure the
function signature and return behavior remain unchanged so tests still assert
the same boolean outcomes.
In `@CONTRIBUTING.md`:
- Line 176: Change the heading text "Full Stack Setup" to the hyphenated form
"Full-Stack Setup" in CONTRIBUTING.md; locate the header line containing the
exact phrase "Full Stack Setup" and update it so the compound adjective uses a
hyphen ("Full-Stack Setup") to match grammatical conventions and static analysis
suggestions.
- Around line 29-42: Update the fenced code block showing the directory tree in
CONTRIBUTING.md by adding a language identifier (e.g., change ``` to ```text)
for the block that starts with "appsmith/"; this satisfies markdownlint rules
for the tree snippet and keeps the visual formatting intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8fe5545-2b59-47db-8ad1-840c135a536f
📒 Files selected for processing (7)
CONTRIBUTING.mdapp/client/src/utils/TypeHelpers.test.tsapp/client/src/utils/URLUtils.test.tsapp/client/src/utils/dayJsUtils.test.tsapp/client/src/utils/formhelpers.test.tsapp/client/src/utils/treeUtils.test.tsapp/client/src/utils/validation/getIsSafeURL.test.ts
Comment on lines
+135
to
+138
| it("should handle array values by converting to string", () => { | ||
| const result = convertObjectToQueryParams({ items: "a,b,c" }); | ||
|
|
||
| expect(result).toBe("?items=a%2Cb%2Cc"); |
Contributor
There was a problem hiding this comment.
Test title does not match what is being tested.
Line 135 says “array values,” but Line 136 passes a plain string ("a,b,c"). This currently validates CSV-string encoding, not array serialization.
💡 Minimal fix
- it("should handle array values by converting to string", () => {
+ it("should handle comma-separated string values", () => {
const result = convertObjectToQueryParams({ items: "a,b,c" });
expect(result).toBe("?items=a%2Cb%2Cc");
});📝 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.
Suggested change
| it("should handle array values by converting to string", () => { | |
| const result = convertObjectToQueryParams({ items: "a,b,c" }); | |
| expect(result).toBe("?items=a%2Cb%2Cc"); | |
| it("should handle comma-separated string values", () => { | |
| const result = convertObjectToQueryParams({ items: "a,b,c" }); | |
| expect(result).toBe("?items=a%2Cb%2Cc"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/utils/URLUtils.test.ts` around lines 135 - 138, The test title
is incorrect: it says "should handle array values..." but passes a CSV string;
update the test to keep behavior or change input. Easiest fix: rename the test
to "should handle CSV string by encoding commas" (in the test that calls
convertObjectToQueryParams({ items: "a,b,c" }) and expects "?items=a%2Cb%2Cc").
Alternatively, if you meant to test array serialization, change the input to an
actual array (e.g., { items: ["a","b","c"] }) and adjust the expected value to
match convertObjectToQueryParams' array serialization behavior.
- Add locale pinning in dayJsUtils.test.ts to ensure deterministic English assertions - Use local Date constructors instead of UTC strings for timezone-independent tests - Fix test description in TypeHelpers.test.ts to match actual behavior
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds comprehensive unit tests for several core utility functions in the Appsmith codebase, improving test coverage and reliability.
Test Coverage Added
New Test Files
Extended Tests
Test Results
All 149 tests pass successfully.
How Has This Been Tested?
Run: npm test -- --testPathPattern="(formhelpers|TypeHelpers|treeUtils|getIsSafeURL|dayJsUtils).test.ts"
Checklist
Summary by CodeRabbit
Documentation
Tests