-
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(smoke): check for passing robots-txt #13007
Conversation
|
||
/** @return {Promise<LH.Artifacts['RobotsTxt']>} */ | ||
/* c8 ignore start */ | ||
async function getRobotsTxtContent() { | ||
try { | ||
const response = await fetch(new URL('/robots.txt', location.href).href); | ||
const response = await fetch(new window.__nativeURL('/robots.txt', location.href).href); |
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.
took awhile to figure out why the smoke test wasn't passing, it's because seo tester html overrides URL
! so, luckily we get a driveby fix here for naughty pages overriding globals.
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.
hm, _cachedNativesPreamble
should be solving this, is something broken there? that's more concerning than the smoke itself :/
lighthouse/lighthouse-core/gather/driver/execution-context.js
Lines 226 to 235 in ce87060
/** | |
* Prefix every script evaluation with a shadowing of common globals that tend to be ponyfilled | |
* incorrectly by many sites. This allows functions to still refer to `Promise` instead of | |
* Lighthouse-specific backups like `__nativePromise` (injected by `cacheNativesOnNewDocument` above). | |
*/ | |
static _cachedNativesPreamble = [ | |
'const Promise = globalThis.__nativePromise || globalThis.Promise', | |
'const URL = globalThis.__nativeURL || globalThis.URL', | |
'const performance = globalThis.__nativePerformance || globalThis.performance', | |
].join(';\n'); |
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.
ok so this seems fine and i can't get it to fail again, weird
@patrickhulce / @adamraine any idea why FR is failing |
Could it be flaky? I'm not seeing a failure locally on the smoke test or a direct FR navigation of the test page. |
would have prevented #12936