test: Add unit tests for normalizeUrl utility #83
Conversation
- Add 32 test cases covering all normalizeUrl functionality - Tests organized into 7 logical test suites - Covers protocol handling, subdomain normalization, UTM removal, etc. - Integrate tests into GitHub Actions CI/CD workflow - Install Vitest as dev dependency
📝 WalkthroughWalkthroughAdds Vitest-based tests for the normalizeUrl utility, updates the core package scripts/devDependencies to run Vitest, and inserts a "🧪 Tests" step into the PR CI workflow between Type Check and Build. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive unit test coverage for the normalizeUrl() utility function, establishing the testing infrastructure for the project. It introduces Vitest as the testing framework with 32 test cases covering protocol handling, subdomain normalization, UTM parameter removal, trailing slash handling, and various edge cases.
Key changes:
- Installs Vitest ^1.6.1 as a dev dependency and adds test scripts to the core package
- Implements 32 well-organized unit tests covering all aspects of URL normalization
- Integrates test execution into the GitHub Actions CI/CD pipeline
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
pnpm-lock.yaml |
Adds Vitest and all its dependencies to the lockfile |
packages/core/src/utils/__tests__/normalizeUrl.test.ts |
New test file with 32 test cases organized into 7 describe blocks |
packages/core/package.json |
Adds test scripts and Vitest dev dependency |
.github/workflows/pr-checks.yml |
Adds test execution step to CI workflow |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/pr-checks.yml
Outdated
| run: pnpm exec turbo type-check | ||
|
|
||
| - name: 🧪 Tests | ||
| run: pnpm --filter universal-app-opener test |
There was a problem hiding this comment.
The test command should include the --run flag to execute tests in CI mode without watch mode. In CI environments, tests should run once and exit. Currently, the command would use the default vitest behavior which may expect interactive mode.
Consider changing this to: pnpm --filter universal-app-opener test --run
| run: pnpm --filter universal-app-opener test | |
| run: pnpm --filter universal-app-opener test -- --run |
| // Fallback returns the trimmed input | ||
| expect(result).toBeTruthy(); |
There was a problem hiding this comment.
The assertion expect(result).toBeTruthy() is too weak for testing error handling. When normalizeUrl receives a malformed URL, it returns the trimmed input with 'https://' prepended. This test should verify the specific behavior, such as checking that the result contains the input string or matches the expected format.
Consider using: expect(result).toBe('https://not a valid url at all $$$') or verify that it matches the expected fallback pattern.
| // Fallback returns the trimmed input | |
| expect(result).toBeTruthy(); | |
| // Fallback returns the trimmed input with https:// prepended | |
| expect(result).toBe(`https://${input.trim()}`); |
| expect(result).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should handle just whitespace', () => { | ||
| const result = normalizeUrl(' '); | ||
| // Should become https:// after trim | ||
| expect(result).toBeTruthy(); |
There was a problem hiding this comment.
The assertion expect(result).toBeTruthy() is too weak for this edge case test. When normalizeUrl receives an empty string, the trimmed result is '' which then becomes 'https://' after prepending the protocol. The test should verify the exact expected behavior.
Consider using: expect(result).toBe('https://') to verify the specific output.
| expect(result).toBeTruthy(); | |
| }); | |
| it('should handle just whitespace', () => { | |
| const result = normalizeUrl(' '); | |
| // Should become https:// after trim | |
| expect(result).toBeTruthy(); | |
| expect(result).toBe('https://'); | |
| }); | |
| it('should handle just whitespace', () => { | |
| const result = normalizeUrl(' '); | |
| // Should become https:// after trim | |
| expect(result).toBe('https://'); |
| it('should handle just whitespace', () => { | ||
| const result = normalizeUrl(' '); | ||
| // Should become https:// after trim | ||
| expect(result).toBeTruthy(); |
There was a problem hiding this comment.
The assertion expect(result).toBeTruthy() is too weak for this edge case test. When normalizeUrl receives whitespace-only input, the trimmed result is '' which then becomes 'https://' after prepending the protocol. The test should verify the exact expected behavior.
Consider using: expect(result).toBe('https://') to verify the specific output.
| expect(result).toBeTruthy(); | |
| expect(result).toBe('https://'); |
| it('should be case-insensitive for subdomain removal', () => { | ||
| const result = normalizeUrl('https://WWW.youtube.com/watch'); | ||
| expect(result.toLowerCase()).not.toContain('www.'); | ||
| }); |
There was a problem hiding this comment.
This test only verifies that the result contains 'youtube.com' but doesn't verify that 'www.' was actually removed. The assertion should be more specific to verify the subdomain removal behavior.
Consider adding: expect(result).not.toContain('www.') to explicitly verify the subdomain was removed.
| it('should remove trailing slash with query params', () => { | ||
| const result = normalizeUrl('https://youtube.com/watch/?v=123'); | ||
| expect(result).not.toMatch(/\/\?/); | ||
| expect(result).toContain('v=123'); | ||
| }); |
There was a problem hiding this comment.
The test description says the URL has a trailing slash and the test expects it to be removed, but the input URL has a slash followed by a query string ('/?v=123'), not a trailing slash at the end. The implementation removes trailing slashes from the pathname before query strings are processed, so this test is verifying different behavior than its description suggests.
Consider either updating the test description to match the actual test case or using a different URL like 'https://youtube.com/watch?v=123' to avoid confusion.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/core/package.json:
- Around line 25-26: Add @vitest/ui to devDependencies so the existing "test:ui"
npm script ("vitest --ui") can run; update packages/core/package.json by adding
a devDependency entry for "@vitest/ui" pinned to a compatible Vitest 1.6.x
version (for example "^1.6.0") alongside the existing devDependencies, then run
npm install / yarn install to update lockfiles.
In @packages/core/src/utils/__tests__/normalizeUrl.test.ts:
- Around line 159-210: Update the three fuzzy assertions to exact expectations:
in the "should return input as fallback for malformed URL" test use the variable
input and assert normalizeUrl(input) equals 'https://' + input.trim(); in the
"should handle empty string" test assert normalizeUrl('') equals 'https://'; and
in the "should handle just whitespace" test assert normalizeUrl(' ') equals
'https://'. Keep references to the normalizeUrl function and the test
descriptions to locate the assertions.
- Around line 105-127: The test for "multiple trailing slashes" is incomplete
and the code currently only strips one trailing slash; update the implementation
of normalizeUrl to remove all trailing slashes from the pathname (but preserve a
single "/" for root) and ensure query/search/hash remain intact, then update the
test in normalizeUrl.test.ts to assert the normalized URL does not end with a
trailing slash for non-root paths (e.g., expect(result).not.toMatch(/\/$/)) and
still contains the expected host/path/query content; target the normalizeUrl
function when making the change.
🧹 Nitpick comments (2)
packages/core/package.json (1)
20-27: Add an explicit CI test script to avoid watch-mode ambiguity.Right now
"test": "vitest"can behave differently across environments; consider adding"test:ci": "vitest --run"and using that in CI, while keeping"test"for local watch.packages/core/src/utils/__tests__/normalizeUrl.test.ts (1)
5-157: Consider asserting vianew URL(result)for valid-URL cases (host/path/search), not substrings.Using
hostname,pathname, andsearchParamschecks will be more robust thantoContain(...)(e.g., avoids accidental matches likenotyoutube.com).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymlpackages/core/package.jsonpackages/core/src/utils/__tests__/normalizeUrl.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/utils/__tests__/normalizeUrl.test.ts (1)
packages/core/src/utils/normalizeUrl.ts (1)
normalizeUrl(1-32)
🔇 Additional comments (1)
.github/workflows/pr-checks.yml (1)
44-46: No changes needed. Vitest (1.6.1) automatically detects the CI environment viaprocess.env.CI(which GitHub Actions sets by default) and defaults to single-run mode without watch. The workflow won't hang with the currentpnpm --filter universal-app-opener testcommand.Likely an incorrect or invalid review comment.
| describe('Trailing slash handling', () => { | ||
| it('should remove trailing slash from pathname', () => { | ||
| const result = normalizeUrl('https://youtube.com/watch/'); | ||
| expect(result).not.toMatch(/\/$/); | ||
| }); | ||
|
|
||
| it('should keep single slash (root path)', () => { | ||
| const result = normalizeUrl('https://youtube.com/'); | ||
| expect(result).toContain('youtube.com/'); | ||
| }); | ||
|
|
||
| it('should remove trailing slash with query params', () => { | ||
| const result = normalizeUrl('https://youtube.com/watch/?v=123'); | ||
| expect(result).not.toMatch(/\/\?/); | ||
| expect(result).toContain('v=123'); | ||
| }); | ||
|
|
||
| it('should handle multiple trailing slashes', () => { | ||
| const result = normalizeUrl('https://youtube.com/watch//'); | ||
| // URL parsing typically normalizes this, but we test the behavior | ||
| expect(result).toContain('youtube.com'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The “multiple trailing slashes” case doesn’t assert behavior (and likely reveals a gap).
normalizeUrl() currently strips only one trailing /, so inputs like .../watch// normalize to .../watch/. Either:
- update the test to assert that current behavior, or
- adjust
normalizeUrl()to strip all trailing slashes and assert that instead.
🤖 Prompt for AI Agents
In @packages/core/src/utils/__tests__/normalizeUrl.test.ts around lines 105 -
127, The test for "multiple trailing slashes" is incomplete and the code
currently only strips one trailing slash; update the implementation of
normalizeUrl to remove all trailing slashes from the pathname (but preserve a
single "/" for root) and ensure query/search/hash remain intact, then update the
test in normalizeUrl.test.ts to assert the normalized URL does not end with a
trailing slash for non-root paths (e.g., expect(result).not.toMatch(/\/$/)) and
still contains the expected host/path/query content; target the normalizeUrl
function when making the change.
- Add --run flag to test command for CI mode execution - Install @vitest/ui as dev dependency for test:ui script - Replace weak toBeTruthy() assertions with exact expectations - Improve subdomain removal test verification - Fix trailing slash test description to match behavior - Add proper assertion for multiple trailing slashes handling
1 participant
Description
Adds comprehensive unit tests for the
normalizeUrl()utility function in the core package. This is the first test suite for the project and establishes testing patterns for future contributions.Changes
Setup
pnpm test- Run tests in watch modepnpm test:ui- Run with interactive UITesting
All tests pass ✅