Skip to content

feat(webdriver-utils): env-var overrides for staging/percy-dev testing #2274

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.

Sign up for GitHub

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

Open
bhokaremoin wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(webdriver-utils): env-var overrides for staging/percy-dev testing #2274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae01e19
feat(webdriver-utils): env-var overrides for staging/percy-dev testing
bhokaremoin Jun 9, 2026
ce10d36
chore(webdriver-utils): drop explanatory comment from browserstackExe…
bhokaremoin Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/webdriver-utils/src/playwrightDriver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ export default class PlaywrightDriver {
command.script = `/* percy_automate_script */ \n ${command.script}`;
}
const options = this.requestPostOptions(command);
const baseUrl = `https://cdp.browserstack.com/wd/hub/session/${this.sessionId}/execute`;
const cdpDomain = process.env.PERCY_CDP_DOMAIN || 'cdp.browserstack.com';
Comment thread
bhokaremoin marked this conversation as resolved.

@bhokaremoin bhokaremoin Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Domain env vars are interpolated without validation

PERCY_CDP_DOMAIN (and PERCY_AUTOMATE_DOMAIN in the providers) is interpolated into the URL unvalidated. Practical risk is very low: the scheme is hardcoded to https:// and these are internal-developer-only vars.

Suggestion: Optional — validate against a hostname pattern if these ever become user-facing.

Reviewer: stack:code-reviewer

const baseUrl = `https://${cdpDomain}/wd/hub/session/${this.sessionId}/execute`;
const response = JSON.parse((await request(baseUrl, options)).body);
return response;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/webdriver-utils/src/providers/automateProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,10 @@ export default class AutomateProvider extends GenericProvider {

async setDebugUrl() {
if (!this.driver) throw new Error('Driver is null, please initialize driver with createDriver().');
const automateDomain = process.env.PERCY_AUTOMATE_DOMAIN || 'automate.browserstack.com';

@bhokaremoin bhokaremoin Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Cache key omits the domain — stale debug URL on repeated setDebugUrl calls

automateDomain is captured by the Cache.withCache closure, but the cache key is only this.driver.sessionId. A second setDebugUrl call for the same session with a different PERCY_AUTOMATE_DOMAIN returns the stale cached URL. Unreachable in real usage (env vars are fixed at process launch), but it's the kind of hazard this staging workflow could trip in test harnesses — the new test already needs Cache.reset() to dodge it.

Suggestion: Drop the Cache.withCache wrapper entirely — the URL is a pure template-string construction with nothing async to cache — or include the domain in the cache key.

Reviewer: stack:code-reviewer

this.debugUrl = await Cache.withCache(Cache.bstackSessionDetails, this.driver.sessionId,
async () => {
return `https://automate.browserstack.com/builds/${this.automateResults.buildHash}/sessions/${this.automateResults.sessionHash}`;
return `https://${automateDomain}/builds/${this.automateResults.buildHash}/sessions/${this.automateResults.sessionHash}`;
});
}

Expand Down
3 changes: 3 additions & 0 deletions packages/webdriver-utils/src/providers/genericProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ export default class GenericProvider {

async browserstackExecutor(action, args) {
if (!this.driver) throw new Error('Driver is null, please initialize driver with createDriver().');
if (action === 'percyScreenshot' && process.env.PERCY_ENABLE_DEV === 'true') {
args = { ...args, projectId: 'percy-dev' };
Comment thread
bhokaremoin marked this conversation as resolved.

@bhokaremoin bhokaremoin Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Spread on possibly-absent args is a latent hazard

If browserstackExecutor('percyScreenshot') were ever called without args, this spread turns a previously argument-less payload into arguments: { projectId: 'percy-dev' }. All current call sites pass an object, so this is latent only.

Suggestion: args = { ...(args || {}), projectId: 'percy-dev' }; to make the intent explicit.

Reviewer: stack:code-reviewer

}
let options = args ? { action, arguments: args } : { action };
let res = await this.driver.executeScript({ script: `browserstack_executor: ${JSON.stringify(options)}`, args: [] });
return res;
Expand Down
3 changes: 2 additions & 1 deletion packages/webdriver-utils/src/providers/playwrightProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ export default class PlaywrightProvider extends GenericProvider {
}

async setDebugUrl() {
this.debugUrl = `https://automate.browserstack.com/builds/${this.automateResults.buildHash}/sessions/${this.automateResults.sessionHash}`;
const automateDomain = process.env.PERCY_AUTOMATE_DOMAIN || 'automate.browserstack.com';
this.debugUrl = `https://${automateDomain}/builds/${this.automateResults.buildHash}/sessions/${this.automateResults.sessionHash}`;
}

async screenshot(name, options) {
Expand Down
12 changes: 12 additions & 0 deletions packages/webdriver-utils/test/playwrightDriver.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@ describe('PlaywrightDriver', () => {
expect(response).toEqual({ value: 'mockVal' });
});

it('should use PERCY_CDP_DOMAIN when set', async () => {
process.env.PERCY_CDP_DOMAIN = 'cdp-k8s-devpercyplat.bsstag.com';
try {
const command = { script: 'console.log("Hello, World!")', args: [] };
await driver.executeScript(command);
const baseUrl = `https://cdp-k8s-devpercyplat.bsstag.com/wd/hub/session/${sessionId}/execute`;
expect(requestSpy).toHaveBeenCalledWith(baseUrl, jasmine.any(Object));
} finally {
delete process.env.PERCY_CDP_DOMAIN;
}
});

it('should handle request error and re-throw', async () => {
requestSpy.and.returnValue(Promise.reject(new Error('Request failed')));
const command = { script: 'console.log("Hello, World!")', args: [] };
Expand Down
13 changes: 13 additions & 0 deletions packages/webdriver-utils/test/providers/automateProvider.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,19 @@ describe('AutomateProvider', () => {
expect(automateProvider.debugUrl).toEqual('https://automate.browserstack.com/builds/12e3/sessions/abc1d');
});

it('uses PERCY_AUTOMATE_DOMAIN when set', async () => {
Cache.reset();

@bhokaremoin bhokaremoin Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Unexplained Cache.reset() hides a test-order dependency

This reset is required because the preceding 'sets automate url' test populates bstackSessionDetails for the same session ID — without it this test would read the stale prod URL from cache. That order-dependence isn't stated anywhere.

Suggestion: Add a one-line comment explaining why the reset is needed.

Reviewer: stack:code-reviewer

process.env.PERCY_AUTOMATE_DOMAIN = 'automate-k8s-mobile.bsstag.com';
try {
let automateProvider = new AutomateProvider(args);
await automateProvider.createDriver();
await automateProvider.screenshot('abc', { });
expect(automateProvider.debugUrl).toEqual('https://automate-k8s-mobile.bsstag.com/builds/12e3/sessions/abc1d');
} finally {
delete process.env.PERCY_AUTOMATE_DOMAIN;
}
});

it('throws error if driver is not initialized', async () => {
let automateProvider = new AutomateProvider(args);
await expectAsync(automateProvider.setDebugUrl())
Expand Down
32 changes: 32 additions & 0 deletions packages/webdriver-utils/test/providers/genericProvider.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,6 +1262,38 @@ describe('GenericProvider', () => {
expect(executeScriptSpy)
.toHaveBeenCalledWith({ script: 'browserstack_executor: {"action":"getSessionDetails","arguments":"new"}', args: [] });
});

describe('with PERCY_ENABLE_DEV', () => {
afterEach(() => {
delete process.env.PERCY_ENABLE_DEV;
});

it('injects projectId: percy-dev into percyScreenshot payloads when enabled', async () => {
process.env.PERCY_ENABLE_DEV = 'true';
let provider = new GenericProvider(args);
await provider.createDriver();
await provider.browserstackExecutor('percyScreenshot', { state: 'begin' });
expect(executeScriptSpy)
.toHaveBeenCalledWith({ script: 'browserstack_executor: {"action":"percyScreenshot","arguments":{"state":"begin","projectId":"percy-dev"}}', args: [] });
});

it('does not inject projectId for non-percyScreenshot actions when enabled', async () => {
process.env.PERCY_ENABLE_DEV = 'true';
let provider = new GenericProvider(args);
await provider.createDriver();
await provider.browserstackExecutor('getSessionDetails', { foo: 'bar' });
expect(executeScriptSpy)
.toHaveBeenCalledWith({ script: 'browserstack_executor: {"action":"getSessionDetails","arguments":{"foo":"bar"}}', args: [] });
});

it('does not inject projectId when disabled (byte-identical to prod)', async () => {
let provider = new GenericProvider(args);
await provider.createDriver();
await provider.browserstackExecutor('percyScreenshot', { state: 'begin' });
expect(executeScriptSpy)
.toHaveBeenCalledWith({ script: 'browserstack_executor: {"action":"percyScreenshot","arguments":{"state":"begin"}}', args: [] });
});
});
});

describe('getTag', () => {
Expand Down
16 changes: 16 additions & 0 deletions packages/webdriver-utils/test/providers/playwrightProvider.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,22 @@ describe('PlaywrightProvider', () => {
'https://automate.browserstack.com/builds/buildHash/sessions/sessionHash'
);
});

it('uses PERCY_AUTOMATE_DOMAIN when set', async () => {
process.env.PERCY_AUTOMATE_DOMAIN = 'automate-k8s-mobile.bsstag.com';
try {
provider.automateResults = {
buildHash: 'buildHash',
sessionHash: 'sessionHash'
};
await provider.setDebugUrl();
expect(provider.debugUrl).toBe(
'https://automate-k8s-mobile.bsstag.com/builds/buildHash/sessions/sessionHash'
);
} finally {
delete process.env.PERCY_AUTOMATE_DOMAIN;
}
});
});

describe('screenshot', () => {
Expand Down
Loading