Skip to content

Commit 15bcd9b

Browse files
jackfranklinDevtools-frontend LUCI CQ
authored andcommitted
Fix reliance on VE Chrome feature flag for VE logs in e2e tests
In crrev.com/c/6734500 the VE Logs feature was removed from Chromium, but we relied on the testing parameter to update the host config for VE Logs for test. This CL updates the code for e2e_non_hosted tests to set a setting `veLogsTestMode` that configures them instead. In a follow-up, we will remove the VE Logs host config from Chromium & DevTools entirely, now that we do not rely on the host config, and it is always enabled, given that the feature flag is now removed and currently hardcoded to `true` on the backend. Bug: none Change-Id: Ib73d9044a32cbdd263891e8f4c24112d293de10b Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6764669 Auto-Submit: Jack Franklin <jacktfranklin@chromium.org> Commit-Queue: Philip Pfaffe <pfaffe@chromium.org> Reviewed-by: Philip Pfaffe <pfaffe@chromium.org>
1 parent 25ca2b9 commit 15bcd9b

5 files changed

Lines changed: 21 additions & 5 deletions

File tree

extensions/cxx_debugging/e2e/MochaRootHooks.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ async function beforeAll() {
4444
`--window-size=${defaultViewport.width + 20, defaultViewport.height + 100}`,
4545
`--custom-devtools-frontend=${new URL(`${DEVTOOLS_DIR}/front_end`, 'file://').href}`,
4646
'--disable-features=RenderDocument',
47-
'--enable-features=DevToolsVeLogging:testing/true',
4847
],
4948
});
5049

@@ -63,6 +62,9 @@ async function beforeAll() {
6362
throw new Error('Could not find frontend page');
6463
}
6564
await frontend.setViewport(defaultViewport);
65+
const devToolsVeLogging = {enabled: true, testing: true};
66+
await frontend.evaluateOnNewDocument(`globalThis.hostConfigForTesting = ${JSON.stringify({devToolsVeLogging})};`);
67+
await frontend.reload();
6668

6769
setBrowserAndPages({frontend, target, browser: conn});
6870
}

front_end/entrypoints/main/MainImpl.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,15 @@ export class MainImpl {
162162

163163
Host.userMetrics.syncSetting(Common.Settings.Settings.instance().moduleSetting<boolean>('sync-preferences').get());
164164
const veLogging = config.devToolsVeLogging;
165+
166+
// Used by e2e_non_hosted to put VE Logs into "test mode".
167+
const veLogsTestMode = Common.Settings.Settings.instance().createSetting('veLogsTestMode', false).get();
168+
165169
if (veLogging?.enabled) {
166-
if (veLogging?.testing) {
170+
// Note: as of https://crrev.com/c/6734500 landing, veLogging.testing is hard-coded to false.
171+
// But the e2e tests (test/conductor/frontend_tab.ts) use this to enable this flag for e2e tests.
172+
// TODO(crbug.com/432411398): remove the host config for VE logs + find a better way to set this up in e2e tests.
173+
if (veLogging?.testing || veLogsTestMode) {
167174
VisualLogging.setVeDebugLoggingEnabled(true, VisualLogging.DebugLoggingFormat.TEST);
168175
const options = {
169176
processingThrottler: new Common.Throttler.Throttler(0),

front_end/ui/visual_logging/KnownContextValues.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1867,6 +1867,7 @@ export const knownContextValues = new Set([
18671867
'is-landscape',
18681868
'is-mobile',
18691869
'is-resident-credential',
1870+
'is-u',
18701871
'is-user-active-false-is-screen-unlocked-false',
18711872
'is-user-active-false-is-screen-unlocked-true',
18721873
'is-user-active-true-is-screen-unlocked-false',
@@ -3926,6 +3927,8 @@ export const knownContextValues = new Set([
39263927
'value-3',
39273928
'vary',
39283929
'vary-header',
3930+
've',
3931+
'veLogsTestMode',
39293932
'vector-effect',
39303933
'verbose',
39313934
'verdana',

test/conductor/frontend_tab.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,9 @@ export class DevToolsFrontendTab {
6060
// We also use a unique ID per DevTools frontend instance, to avoid the same issue with other
6161
// frontend instances.
6262
const id = DevToolsFrontendTab.tabCounter++;
63+
6364
const frontendUrl = `https://i${id}.devtools-frontend.test:${testServerPort}/${devToolsAppURL}?ws=localhost:${
64-
getDebugPort(browser)}/devtools/page/${targetId}&targetType=tab&veLogging=true`;
65+
getDebugPort(browser)}/devtools/page/${targetId}&targetType=tab`;
6566

6667
const frontend = await browser.newPage();
6768
installPageErrorHandlers(frontend);

test/e2e_non_hosted/shared/frontend-helper.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -601,7 +601,7 @@ export interface DevtoolsSettings {
601601
export const DEFAULT_DEVTOOLS_SETTINGS: DevtoolsSettings = {
602602
enabledDevToolsExperiments: [],
603603
devToolsSettings: {
604-
isUnderTest: true,
604+
veLogsTestMode: true,
605605
},
606606
dockingMode: 'right',
607607
};
@@ -621,7 +621,10 @@ async function setDevToolsSettings(devToolsPata: DevToolsPage, settings: Record<
621621
return await devToolsPata.evaluate(`(async () => {
622622
const Common = await import('./core/common/common.js');
623623
${rawValues.map(([settingName, value]) => {
624-
return `Common.Settings.Settings.instance().createSetting('${settingName}', ${value});`;
624+
// Creating the setting might not be enough if it already exists, so we
625+
// create it and then forcibly set the value.
626+
return `const setting = Common.Settings.Settings.instance().createSetting('${settingName}', ${value});
627+
setting.set(${value});`;
625628
})}
626629
})()`);
627630
}

0 commit comments

Comments
 (0)