core: use the main frame url for finalUrl#13819
Conversation
| driver = new EmulationDriver(connectionStub); | ||
| resetDefaultMockResponses(); | ||
|
|
||
| fakeDriver.url = jest.fn().mockResolvedValue('about:blank'); |
There was a problem hiding this comment.
Some tests use fakeDriver, some use driver, some make their own driver 🙃
| // Note that the final URL is the URL of the network requested resource and not that page we end up on. | ||
| // http://localhost:10200/push-state | ||
| finalUrl: 'http://localhost:10200/redirects-final.html?pushState', | ||
| finalUrl: 'http://localhost:10200/push-state', |
|
😬 awkward to bring this up long after you opened #13706: what do you think of a rename to something else for the final frame url? HTTP Archive analysis out there in particular is really reliant on Since things like that aren't using Lighthouse directly (and are unlikely to read the changelog), will probably be copy/pasted into future analysis scripts, and everything will still appear to work since often
|
|
can you add a smoke test / scenario test that asserts that the URL hash is present in final url? |
|
I'm +1 on keeping finalUrl and introducing a new |
|
me connor adam discussed and are gravitating towards making this a very BREAKING change:
all clients have to choose what they want for each instance |
|
I prefer option 2 because I want to avoid introducing permanent confusion (keeping My preference:
This forces users of |
|
I think I'm going to merge this pending connor's request for a smoke test and do a rename of |
| # errors-expired-ssl errors-infinite-loop | ||
| # Disabled because Chrome will follow the redirect first, and Lighthouse will only ever see/run the final URL. | ||
| # redirects-client-paint-server redirects-multiple-server redirects-single-server redirects-single-client | ||
| # redirects-client-paint-server redirects-multiple-server redirects-single-server redirects-single-client redirects-hash |
There was a problem hiding this comment.
Perhaps our test definitions should begin to declare themselves with excludeRunners.
There was a problem hiding this comment.
Otherwise local usage of the runner will fail unless you remember to disable these tests :)
I'll stress again that this is really annoying for people consuming LHRs :) Another use case: comparing HTTP Archive results over time. The standard query is going to have use conditionals on the Lighthouse version to get a joinable URL. Dropping it from the
I still think some kind of |
| @@ -0,0 +1,34 @@ | |||
| /** | |||
There was a problem hiding this comment.
smoke tests are slow, is it possible to combine this with an existing smoke test if it's just the hash we're after? It doesn't need to use pushState, the requestedUrl could even include a hash and that wouldn't currently end up in finalUrl
There was a problem hiding this comment.
I did the client and server redirects test because the hash is not preserved over a client redirect (without adding it yourself) but it is preserved over a server redirect.
|
I'm going to try and organize our bikeshedding issues in the parent issue for this. |
Step 5 of #13706
I did a pass on all usages of
finalUrlin the report renderer and treemap. Every usage in the report renderer was either cosmetic or just looking at the origin, so we should be fine there.In the treemap, we use the
finalUrlto determine if a first-level node is for inline html scripts. With the changes in #13802 (comment) we can determine this by looking for "(inline)" prefixes in the children, however we should keep the URL check around as a backport.