-
Notifications
You must be signed in to change notification settings - Fork 30
Add Edge and Chrome Browser Support #7
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
base: main
Are you sure you want to change the base?
Conversation
…e, Chrome, and Chromium support
…te package.json to enable npm test scripts - Adjust .prettierrc printWidth to 120 to minimize excessive formatting changes; previous settings caused nearly every line to be reformatted, making diffs hard to review. - Update package.json to add npm scripts for running tests, enabling easier multi-browser test execution.
- Prettier was configured but appeared to have never been run on the project. - This commit applies formatting using the already installed Prettier, resulting in whitespace-only changes across all files. - No functional or logic changes.
Co-authored-by: Copilot <[email protected]>
|
@mrkrsl I couldn't find a specific PR guideline in your codebase so just tried to make the code generally of good quality and pass the linters and tests (which I extended to cover my code). Please let me know if you have a specific process I should follow. |
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 additional browser support for Edge and Chrome browsers, enhances the test suite with multi-browser testing capabilities, and applies consistent Prettier formatting across the codebase.
- Multi-Browser Support: Adds Chrome and Edge browser channels through Playwright configuration and testing infrastructure
- Test Infrastructure Improvements: Creates reusable test utilities for running tests across multiple browsers and consolidates test execution
- Code Formatting: Applies Prettier formatting consistently across all files with updated configuration
Reviewed Changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test-utilities.js | Adds multi-browser test runner utility with browser installation checking |
| tests/test-search.js | Updates search test to use multi-browser testing framework |
| tests/test-duckduckgo.js | Refactors DuckDuckGo test for multi-browser support |
| tests/test-brave.js | Refactors Brave test for multi-browser support |
| tests/test-bing.js | Refactors Bing test for multi-browser support |
| tests/test-all-engines.js | Updates comprehensive test suite for multi-browser support |
| tests/run-all-tests.js | Adds test runner to execute all test modules sequentially |
| tests/browser-installed.js | Utility for checking browser installation status |
| src/search-engine.ts | Adds Chrome/Edge browser support and applies formatting |
| src/browser-pool.ts | Extends browser pool to support Edge and Chrome channels |
| Other files | Applies Prettier formatting without functional changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| * and returns a Promise resolving to true (pass) or false (fail). | ||
| * @param {string} label - A label describing the test, used in output messages. | ||
| */ | ||
| export async function runMultiBrowserTest(testFn, label) { |
Copilot
AI
Aug 17, 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.
The documentation states the function exits the process, but this behavior could be problematic when used as a library function. Consider documenting this as a limitation or providing a non-exiting variant.
| export async function runMultiBrowserTest(testFn, label) { | |
| * By default, returns true if all tests pass, or false if any test fails. | |
| * Optionally, if `shouldExit` is true, exits the process with code 0 (pass) or 1 (fail). | |
| * | |
| * @param {(channel: string|undefined) => Promise<boolean>} testFn - | |
| * An async callback that receives the browser channel name (or undefined for default Chromium) | |
| * and returns a Promise resolving to true (pass) or false (fail). | |
| * @param {string} label - A label describing the test, used in output messages. | |
| * @param {boolean} [shouldExit=false] - If true, exits the process after running tests. | |
| */ | |
| export async function runMultiBrowserTest(testFn, label, shouldExit = false) { |
| } | ||
|
|
||
| console.log(`\n${label} RESULT: ${allPassed ? 'WORKING ✅' : 'FAILED ❌'}`); | ||
| process.exit(allPassed ? 0 : 1); |
Copilot
AI
Aug 17, 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.
The function always calls process.exit(), which makes it unusable as a library function and could interfere with test runners or other calling code. Consider returning the result instead and letting the caller decide whether to exit.
| process.exit(allPassed ? 0 : 1); | |
| return allPassed; |
| for (const file of testFiles) { | ||
| console.log(`\n[run-all-tests] Running: ${file}`); | ||
| try { | ||
| execSync(`node ${file}`, { stdio: 'inherit' }); |
Copilot
AI
Aug 17, 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.
Using execSync with string interpolation could be vulnerable to command injection if file paths contain special characters. Consider using an array format or validating the file paths.
| execSync(`node ${file}`, { stdio: 'inherit' }); | |
| execSync('node', [file], { stdio: 'inherit' }); |
| // Create a dedicated browser instance for Bing search only | ||
| const { chromium } = await import('playwright'); | ||
| const startTime = Date.now(); | ||
| // Support launching Chrome and Edge as branded channels |
Copilot
AI
Aug 17, 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.
The comment mentions 'branded channels' but the implementation only checks environment variables. Consider updating the comment to accurately describe the environment variable-based channel selection.
| // Support launching Chrome and Edge as branded channels | |
| // Launch Chrome or Edge as branded channels if BROWSER_TYPE environment variable is set to 'chrome' or 'edge' |
| let channel: string | undefined; | ||
| const browserType = process.env.BROWSER_TYPE || null; | ||
| if (browserType === 'chrome') channel = 'chrome'; | ||
| if (browserType === 'edge') channel = 'msedge'; |
Copilot
AI
Aug 17, 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.
The channel selection logic is duplicated in this file and in browser-pool.ts. Consider extracting this logic into a shared utility function to avoid duplication.
| if (browserType === 'edge') channel = 'msedge'; | |
| const channel = getBrowserChannel(); |
Overview
This PR introduces additional browser support (Edge, Chrome), expands the test suite, and applies Prettier formatting across the codebase.
What this provides and why
Commit summary
.prettierrcandpackage.jsonfor better formatting and test execution.Review guidance
Build & test status