-
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
core: warn if document was redirected #10157
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,9 @@ const expectations = [ | |
}, | ||
}, | ||
}, | ||
runWarnings: [ | ||
/The page may not be loading as expected because your test URL \(.*online-only.html.*\) was redirected to .*redirects-final.html. Try testing the second URL directly./, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a non-deterministic component of this that warrants a regex? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this first one - yes, |
||
], | ||
}, | ||
}, | ||
{ | ||
|
@@ -44,6 +47,9 @@ const expectations = [ | |
}, | ||
}, | ||
}, | ||
runWarnings: [ | ||
/The page may not be loading as expected because your test URL \(.*online-only.html.*\) was redirected to .*redirects-final.html. Try testing the second URL directly./, | ||
], | ||
}, | ||
}, | ||
]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,20 @@ const NetworkAnalyzer = require('../lib/dependency-graph/simulator/network-analy | |
const NetworkRecorder = require('../lib/network-recorder.js'); | ||
const constants = require('../config/constants.js'); | ||
const i18n = require('../lib/i18n/i18n.js'); | ||
const URL = require('../lib/url-shim.js'); | ||
|
||
const UIStrings = { | ||
/** | ||
* @description Warning that the web page redirected during testing and that may have affected the load. | ||
* @example {https://example.com/requested/page} requested | ||
* @example {https://example.com/final/resolved/page} final | ||
*/ | ||
warningRedirected: 'The page may not be loading as expected because your test URL ' + | ||
`({requested}) was redirected to {final}. ` + | ||
'Try testing the second URL directly.', | ||
}; | ||
|
||
const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); | ||
|
||
/** @typedef {import('../gather/driver.js')} Driver */ | ||
|
||
|
@@ -491,6 +505,13 @@ class GatherRunner { | |
|
||
// Copy redirected URL to artifact. | ||
baseArtifacts.URL.finalUrl = passContext.url; | ||
/* eslint-disable max-len */ | ||
if (!URL.equalWithExcludedFragments(baseArtifacts.URL.requestedUrl, baseArtifacts.URL.finalUrl)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #10218 is going to throw a major wrench into all the places where we detect redirects like this 😆 but a problem for another day |
||
baseArtifacts.LighthouseRunWarnings.push(str_(UIStrings.warningRedirected, { | ||
requested: baseArtifacts.URL.requestedUrl, | ||
final: baseArtifacts.URL.finalUrl, | ||
})); | ||
} | ||
|
||
// Fetch the manifest, if it exists. | ||
baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); | ||
|
@@ -672,3 +693,4 @@ class GatherRunner { | |
} | ||
|
||
module.exports = GatherRunner; | ||
module.exports.UIStrings = UIStrings; |
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.
This does work offline. Was this meant to be testing something subtle re: redirection? I'm assuming not, since no comments.
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.
I'm guessing it didn't always work offline, this test might be 4 years old :)