test: add selector validation edge case tests#1072
Open
okwn wants to merge 1 commit into
Open
Conversation
Add comprehensive tests for CSS selector validation edge cases: - Empty string selector - Very long selector strings - Special characters that could break DOM - Unicode characters in selectors - XPath selectors - Selectors with quotes, whitespace, newlines Risk: LOW - tests only
WalkthroughA new Jest test file adds comprehensive test coverage for ChangesSelectorValidator test coverage
🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/sdk/__tests__/selector-validation.test.ts`:
- Around line 41-47: The current tests in selector-validation.test.ts only
assert the shape of the validator.validateSelector result; update each affected
test (e.g., the case using longSelector and other cases around validateSelector
calls at the mentioned blocks) to assert concrete expectations instead of just
keys: check result.valid is true or false as appropriate for the input, and
assert error is null/undefined for valid cases or contains the expected
message/substring/code for invalid cases; locate calls to
validator.validateSelector and variables like longSelector and replace the
generic expect(result).toHaveProperty(...) assertions with explicit expects on
result.valid and result.error semantics to validate correct behavior for each
edge case.
- Around line 9-22: The locator mock in createMockPage always returns the same
mockLocator, so selector-specific failures aren’t exercised; update
createMockPage so the locator jest.fn() accepts a selector argument and returns
different mockLocator objects or behaviors based on that selector (e.g., return
a mockLocator whose count() resolves 0 or whose evaluate() throws for invalid
selectors, and a successful mockLocator for valid selectors). Reference
createMockPage, locator, mockLocator, count, and evaluate when implementing
selector-aware branching so tests for malformed selectors hit the failure paths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9de5ecc0-be4d-478c-b84c-007543c040bc
📒 Files selected for processing (1)
server/src/sdk/__tests__/selector-validation.test.ts
Comment on lines
+9
to
+22
| const createMockPage = () => { | ||
| const mockLocator = { | ||
| first: () => mockLocator, | ||
| count: jest.fn().mockResolvedValue(0), | ||
| evaluate: jest.fn().mockResolvedValue('DIV'), | ||
| }; | ||
| return { | ||
| locator: jest.fn().mockReturnValue(mockLocator), | ||
| goto: jest.fn().mockResolvedValue(undefined), | ||
| waitForTimeout: jest.fn().mockResolvedValue(undefined), | ||
| click: jest.fn().mockResolvedValue(undefined), | ||
| waitForSelector: jest.fn().mockResolvedValue(undefined), | ||
| evaluate: jest.fn().mockResolvedValue(undefined), | ||
| }; |
There was a problem hiding this comment.
Make locator mock selector-aware to avoid false positives.
Right now (Line 16) every selector returns the same successful locator path, so malformed selector tests don’t exercise failure behavior at all.
Proposed adjustment
- locator: jest.fn().mockReturnValue(mockLocator),
+ locator: jest.fn((selector: string) => {
+ if (!selector.trim()) throw new Error('Invalid selector');
+ if (selector.includes('<script>') || selector.includes('::before{')) {
+ throw new Error('Invalid selector');
+ }
+ return mockLocator;
+ }),📝 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
| const createMockPage = () => { | |
| const mockLocator = { | |
| first: () => mockLocator, | |
| count: jest.fn().mockResolvedValue(0), | |
| evaluate: jest.fn().mockResolvedValue('DIV'), | |
| }; | |
| return { | |
| locator: jest.fn().mockReturnValue(mockLocator), | |
| goto: jest.fn().mockResolvedValue(undefined), | |
| waitForTimeout: jest.fn().mockResolvedValue(undefined), | |
| click: jest.fn().mockResolvedValue(undefined), | |
| waitForSelector: jest.fn().mockResolvedValue(undefined), | |
| evaluate: jest.fn().mockResolvedValue(undefined), | |
| }; | |
| const createMockPage = () => { | |
| const mockLocator = { | |
| first: () => mockLocator, | |
| count: jest.fn().mockResolvedValue(0), | |
| evaluate: jest.fn().mockResolvedValue('DIV'), | |
| }; | |
| return { | |
| locator: jest.fn((selector: string) => { | |
| if (!selector.trim()) throw new Error('Invalid selector'); | |
| if (selector.includes('<script>') || selector.includes('::before{')) { | |
| throw new Error('Invalid selector'); | |
| } | |
| return mockLocator; | |
| }), | |
| goto: jest.fn().mockResolvedValue(undefined), | |
| waitForTimeout: jest.fn().mockResolvedValue(undefined), | |
| click: jest.fn().mockResolvedValue(undefined), | |
| waitForSelector: jest.fn().mockResolvedValue(undefined), | |
| evaluate: jest.fn().mockResolvedValue(undefined), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/sdk/__tests__/selector-validation.test.ts` around lines 9 - 22,
The locator mock in createMockPage always returns the same mockLocator, so
selector-specific failures aren’t exercised; update createMockPage so the
locator jest.fn() accepts a selector argument and returns different mockLocator
objects or behaviors based on that selector (e.g., return a mockLocator whose
count() resolves 0 or whose evaluate() throws for invalid selectors, and a
successful mockLocator for valid selectors). Reference createMockPage, locator,
mockLocator, count, and evaluate when implementing selector-aware branching so
tests for malformed selectors hit the failure paths.
Comment on lines
+41
to
+47
| it('should handle very long selector strings', async () => { | ||
| const longSelector = 'div'.repeat(10000); | ||
| // Should not throw, should return valid or invalid gracefully | ||
| const result = await validator.validateSelector({ selector: longSelector }); | ||
| expect(result).toHaveProperty('valid'); | ||
| expect(result).toHaveProperty('error'); | ||
| }); |
There was a problem hiding this comment.
Strengthen assertions to validate behavior, not just object shape.
These checks mostly verify keys exist, so they don’t confirm correctness for each edge case. Please assert explicit outcomes (valid true/false and expected error semantics) per scenario.
Example assertion upgrade pattern
- expect(result).toHaveProperty('valid');
- expect(result).toHaveProperty('error');
+ expect(result.valid).toBe(true);
+ expect(result.error).toBeUndefined();- expect(result).toHaveProperty('valid');
- expect(result).toHaveProperty('error');
+ expect(result.valid).toBe(false);
+ expect(result.error).toBeDefined();Also applies to: 57-63, 74-79, 95-97, 104-107, 114-116, 123-125, 132-133, 140-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/sdk/__tests__/selector-validation.test.ts` around lines 41 - 47,
The current tests in selector-validation.test.ts only assert the shape of the
validator.validateSelector result; update each affected test (e.g., the case
using longSelector and other cases around validateSelector calls at the
mentioned blocks) to assert concrete expectations instead of just keys: check
result.valid is true or false as appropriate for the input, and assert error is
null/undefined for valid cases or contains the expected message/substring/code
for invalid cases; locate calls to validator.validateSelector and variables like
longSelector and replace the generic expect(result).toHaveProperty(...)
assertions with explicit expects on result.valid and result.error semantics to
validate correct behavior for each edge case.
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
2 participants
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.
Summary
Problem
Solution
Tests
npm test -- --testPathPattern=selector-validationRisk / Compatibility
Summary by CodeRabbit