Skip to content

tests(smoke): check for passing robots-txt #13007

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

Jump to bottom
Merged
devtools-bot merged 4 commits into from
Sep 3, 2021
Merged

tests(smoke): check for passing robots-txt #13007

devtools-bot merged 4 commits into from
Sep 3, 2021

Conversation

@connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Sep 2, 2021

would have prevented #12936

@connorjclark connorjclark requested a review from a team as a code owner September 2, 2021 21:25
@connorjclark connorjclark requested review from patrickhulce and removed request for a team September 2, 2021 21:25
@google-cla google-cla bot added the cla: yes label Sep 2, 2021
connorjclark
connorjclark commented Sep 2, 2021
View reviewed changes
lighthouse-core/gather/gatherers/seo/robots-txt.js Outdated
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);
Copy link
Collaborator Author

@connorjclark connorjclark Sep 2, 2021

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.

Copy link
Collaborator

@patrickhulce patrickhulce Sep 2, 2021

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');

Copy link
Collaborator Author

@connorjclark connorjclark Sep 2, 2021

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

@connorjclark connorjclark changed the title test(smoke): check for passing robots-txt tests(smoke): check for passing robots-txt Sep 2, 2021
@connorjclark
Copy link
Collaborator Author

connorjclark commented Sep 3, 2021 •
edited
Loading

@patrickhulce / @adamraine any idea why FR is failing seo-passing / to fetch robots.txt?

@adamraine
Copy link
Contributor

adamraine commented Sep 3, 2021

Could it be flaky? I'm not seeing a failure locally on the smoke test or a direct FR navigation of the test page.

@devtools-bot devtools-bot merged commit d6225d3 into master Sep 3, 2021
@devtools-bot devtools-bot deleted the robots-smoke branch September 3, 2021 23:09
satya-nutella pushed a commit to satya-nutella/lighthouse that referenced this pull request Sep 7, 2021
@connorjclark @satya-nutella
tests(smoke): check for passing robots-txt (GoogleChrome#13007)
8034033
@connorjclark connorjclark mentioned this pull request Mar 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@patrickhulce patrickhulce patrickhulce approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@patrickhulce patrickhulce

Labels

cla: yes waiting4committer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@connorjclark @adamraine @patrickhulce @devtools-bot