Conversation
There was a problem hiding this comment.
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('#')); |
There was a problem hiding this comment.
[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); | ||
| }); |
There was a problem hiding this comment.
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([]); |
There was a problem hiding this comment.
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.