-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
tests(devtools): support dbw smoke #14616
Conversation
// 22 requests made for a single navigation. | ||
// 6 extra requests made because stylesheets are evicted from the cache by the time DT opens. | ||
length: 28, | ||
}, |
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.
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.
Nice! Did not remember dbw was in the exclusion list. We should look into those "need further investigation" ones at some point too.
@@ -315,10 +322,14 @@ async function testUrlFromDevtools(url, options = {}) { | |||
const logs = []; | |||
await installConsoleListener(inspectorSession, logs); | |||
|
|||
page.on('dialog', dismissDialog); |
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 need for this means
lighthouse/core/gather/driver/prepare.js
Lines 51 to 70 in 1738b1a
/** | |
* Dismiss JavaScript dialogs (alert, confirm, prompt), providing a | |
* generic promptText in case the dialog is a prompt. | |
* @param {LH.Gatherer.FRProtocolSession} session | |
* @return {Promise<void>} | |
*/ | |
async function dismissJavaScriptDialogs(session) { | |
session.on('Page.javascriptDialogOpening', data => { | |
log.warn('Driver', `${data.type} dialog opened by the page automatically suppressed.`); | |
session | |
.sendCommand('Page.handleJavaScriptDialog', { | |
accept: true, | |
promptText: 'Lighthouse prompt response', | |
}) | |
.catch(err => log.warn('Driver', err)); | |
}); | |
await session.sendCommand('Page.enable'); | |
} |
Lots of previous history
- handleJavascriptDialogs (#1939) #2106
- handle multiple javascript modal prompts #2120
- handle Page.handleJavaScriptDialog rejection #2402
- Lighthouse fails to dismiss page dialogs #2423
- Disable the dismissJavaScriptDialogs smoketest. #2437
- core(driver): recover from rejection on handleJavaScriptDialog #6327
- tests(smoke): re-enable dialog prompt #8894
I guess we should open an issue?
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.
It is working in DevTools. In the setup for the test however, the dialog prevents pptr-run-devtools.js
from starting the Lighthouse run. This PR just dismissed the dialog in the initial test setup so we can actually start the test. The dialog dismiss listener is then removed before the test starts.
During the test, the page is reloaded so the dialog appears again. This time the code you linked is responsible for dismissing the dialog.
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.
Ah, I missed that this was running only before the test. Probably could use a comment but at least blame will find this :)
Would be nice to have DT smoke coverage of #14465. Turns out it was pretty easy to get dbw working in the DT smoke runner.