Skip to content

Conversation

@Vality
Copy link

@Vality Vality commented Aug 17, 2025

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

  • Multi-Browser Support: Enables the project to run and be tested on Edge and Chrome, (in additon to the existing Chromium and Firefox support), increasing compatibility, these browsers tend to be less likely to be detected as bots, these new browsers are not used by default but can be configured in the mcp config.
  • Test Improvements: Refactors and extends the test suite to cover the new browsers, catching browser-specific issues.
  • Formatting Cleanup: Applies Prettier formatting project-wide for consistency. Prettier was configured but had apparently never been run, resulting in inconsistent formatting across the codebase.

Commit summary

  1. Edge/Chrome Support: Adds core logic and configuration for multi-browser support.
  2. Test Improvements: Updates and expands test scripts/utilities for new supported browsers.
  3. Project Config Updates: Updates .prettierrc and package.json for better formatting and test execution.
  4. Formatting-Only Commit: Runs Prettier for the first time, resulting in whitespace-only changes.
  5. package-lock.json Update: Updates lockfile after a full build/test cycle.

Review guidance

  • Please review the functional and test changes carefully, as they introduce new browser support and test logic.
  • Formatting-only changes are isolated in a single commit and only contain automated prettier changes so will not affect logic.

Build & test status

  • Full build and test cycle completed.
  • All tests passed for Chromium and Edge.
  • Only failure experienced was intermittent DuckDuckGo bot detection during some test runs (not related to code or changes, happened both before and after changes).

Vality added 5 commits August 16, 2025 18:56
…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.
Copilot AI review requested due to automatic review settings August 17, 2025 01:44

This comment was marked as outdated.

@Vality
Copy link
Author

Vality commented Aug 17, 2025

@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.

@Vality Vality requested a review from Copilot August 17, 2025 04:08
Copy link

Copilot AI left a 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) {
Copy link

Copilot AI Aug 17, 2025

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.

Suggested change
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) {

Copilot uses AI. Check for mistakes.
}

console.log(`\n${label} RESULT: ${allPassed ? 'WORKING ✅' : 'FAILED ❌'}`);
process.exit(allPassed ? 0 : 1);
Copy link

Copilot AI Aug 17, 2025

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.

Suggested change
process.exit(allPassed ? 0 : 1);
return allPassed;

Copilot uses AI. Check for mistakes.
for (const file of testFiles) {
console.log(`\n[run-all-tests] Running: ${file}`);
try {
execSync(`node ${file}`, { stdio: 'inherit' });
Copy link

Copilot AI Aug 17, 2025

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.

Suggested change
execSync(`node ${file}`, { stdio: 'inherit' });
execSync('node', [file], { stdio: 'inherit' });

Copilot uses AI. Check for mistakes.
// 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
Copy link

Copilot AI Aug 17, 2025

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.

Suggested change
// 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'

Copilot uses AI. Check for mistakes.
let channel: string | undefined;
const browserType = process.env.BROWSER_TYPE || null;
if (browserType === 'chrome') channel = 'chrome';
if (browserType === 'edge') channel = 'msedge';
Copy link

Copilot AI Aug 17, 2025

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.

Suggested change
if (browserType === 'edge') channel = 'msedge';
const channel = getBrowserChannel();

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant