-
Notifications
You must be signed in to change notification settings - Fork 0
✨ globalIgnores and improved documentation #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces global ignore functionality and improves documentation for the playwright-addon package. It adds the ability to configure global ignore patterns in the Playwright config that apply across all tests, complementing the existing per-test ignore files.
Key changes:
- Added global ignore support through project metadata configuration
- Enhanced filtering logic to support both exact matches and regex patterns
- Improved documentation with global ignore configuration examples and setup instructions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/functions/snapshotTest.ts | Added clock resume call before accessibility check |
| src/functions/ignoreFile.ts | Implemented global ignore functionality with regex support and improved filtering logic |
| src/functions/consoleLogging.ts | Added empty assertion for update mode |
| src/functions/checkAccessibility.ts | Removed commented-out clock resume call |
| README.md | Enhanced documentation with global ignore configuration and setup instructions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| .map((line: string) => line.trim()) | ||
| .filter((line: string) => line && !line.startsWith('#')); |
Copilot
AI
Sep 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The explicit type annotations on the callback parameters are unnecessary since TypeScript can infer the types from the array context. Consider removing : string from both callbacks for cleaner code.
| .map((line: string) => line.trim()) | |
| .filter((line: string) => line && !line.startsWith('#')); | |
| .map((line) => line.trim()) | |
| .filter((line) => line && !line.startsWith('#')); |
| const ignoresSet = new Set([...globalIgnores, ...ignores]); | ||
| const allIgnores = [...ignoresSet].map((ignore) => { | ||
| if (typeof ignore !== 'string') { | ||
| return ignore; | ||
| } | ||
| if (!ignore.startsWith('^') && !ignore.endsWith('$')) { | ||
| return ignore; | ||
| } | ||
| return new RegExp(ignore); | ||
| }); |
Copilot
AI
Sep 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a Set from mixed types (string | RegExp) and then converting back to an array defeats the purpose of deduplication, as RegExp objects with identical patterns are different object references. This could lead to duplicate regex patterns in the final array.
| const ignoresSet = new Set([...globalIgnores, ...ignores]); | |
| const allIgnores = [...ignoresSet].map((ignore) => { | |
| if (typeof ignore !== 'string') { | |
| return ignore; | |
| } | |
| if (!ignore.startsWith('^') && !ignore.endsWith('$')) { | |
| return ignore; | |
| } | |
| return new RegExp(ignore); | |
| }); | |
| // Deduplicate ignores: strings by value, RegExp by source+flags | |
| const seenStrings = new Set<string>(); | |
| const seenRegexes = new Set<string>(); | |
| const allIgnores: (string | RegExp)[] = []; | |
| for (const ignore of [...globalIgnores, ...ignores]) { | |
| if (typeof ignore === 'string') { | |
| // If string looks like a regex, convert to RegExp | |
| if (ignore.startsWith('^') || ignore.endsWith('$')) { | |
| const regexKey = ignore + ''; | |
| if (!seenRegexes.has(regexKey)) { | |
| seenRegexes.add(regexKey); | |
| allIgnores.push(new RegExp(ignore)); | |
| } | |
| } else { | |
| if (!seenStrings.has(ignore)) { | |
| seenStrings.add(ignore); | |
| allIgnores.push(ignore); | |
| } | |
| } | |
| } else if (ignore instanceof RegExp) { | |
| const regexKey = ignore.source + '|' + ignore.flags; | |
| if (!seenRegexes.has(regexKey)) { | |
| seenRegexes.add(regexKey); | |
| allIgnores.push(ignore); | |
| } | |
| } | |
| } |
| const update = ['all', 'changed'].includes(test.info().config.updateSnapshots); | ||
| if (update) { | ||
| await writeIgnoreFile(logs, '.consoleignore'); | ||
| expect([], 'no console outputs').toEqual([]); |
Copilot
AI
Sep 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion will always pass since an empty array equals an empty array. This appears to be a placeholder or debugging code that should be removed or replaced with meaningful logic.
| expect([], 'no console outputs').toEqual([]); | |
| expect(logs, 'no console outputs').toEqual([]); |
No description provided.