tests: smoke request count assertion#12325
Conversation
| requestCountAssertion.push(makeComparison( | ||
| 'Request count', | ||
| server.getRequestUrls(urlPath), | ||
| expected.networkRequests |
There was a problem hiding this comment.
i think it'd be good to exercise the actual assertions within the PR.
There was a problem hiding this comment.
I added some expectations to perf.
There was a problem hiding this comment.
I added some expectations to perf.
dbw might be one of the best to add expectations to since the page tries to be bad at everything and the smoke test runs the full default config. It's also just the one smoke test, so shouldn't be much work to add.
There was a problem hiding this comment.
dbw is two smoke tests run in parallel, one smoke test is an external URL.
The smoke test should still work if we adjust our requirement from "Tests run synchronously" to "Only one test using the static server at a time". Definitely doable, but it might add a bit of complexity.
There was a problem hiding this comment.
Ah, yeah, I meant just the first one anyways. Agreed it's not worth the complexity (at least for now) to add networkRequests to the second one
There was a problem hiding this comment.
That is unfortunately still a problem. Under the current implementation, the expectation would be pruned/throw because dbw is run in parallel. We need to change the definition (adding complexity) for the expectation to be actually be asserted.
There was a problem hiding this comment.
Oh, I see, you're talking about runSerially so we've been talking about slightly different things this whole time, sorry :/
You have the isSync value based on concurrency, which is the combination of runSerially with the --jobs flag/option:
lighthouse/lighthouse-cli/test/smokehouse/smokehouse.js
Lines 55 to 56 in 802947d
so it'll be pruned in a the usual local runs, but in CI we run with -j=1 for vm performance reasons (and maybe still coverage collection issues?) so concurrency will still be 1 and it shouldn't be pruned there.
There was a problem hiding this comment.
Now that I write that, maybe it'll be annoying when editing dbw to only find out about failures in CI unless you happen to run with -j=1 🤷
If ever there was a smoke test perfect for #11506 it would be dbw, though. Should we just runSerially: true it and take the hit on slower smoke tests but keep things simpler?
There was a problem hiding this comment.
Should we just
runSerially: trueit and take the hit on slower smoke tests but keep things simpler?
That's what I'm thinking, it shouldn't be too bad because there are only two tests.
|
So it looks like grouping requests by socket doesn't work 100% of the time. Unfortunately, I'm not sure how else to group requests by test. Any ideas on how to proceed? |
Co-authored-by: Connor Clark <cjamcl@google.com>
I swear I also offered some hesitation about this in my review, but don't see that text now. Anyway, I agree that socket probably won't map to test case well.
One idea.. cookies. But tbh i don't like it all. If we restrict these assertions to only work on |
Ok, I think this is the best option to pursue for now. |
| if (expected.networkRequests) { | ||
| requestCountAssertion.push(makeComparison( | ||
| 'Requests', | ||
| server.getRequestUrls(), |
There was a problem hiding this comment.
it seems like this should be run outside of the assert module and the result passed in, like the other parts of actual. smokehouse.js is a natural place to do that, but that wouldn't work for the bundled version run for smokerider (which doesn't use static-server). Almost needs to be part of SmokehouseOptions, similar to the lighthouseRunner callback?
(I don't love that idea, but I'm not sure of a better one)
| isDebug, | ||
| })); | ||
| const individualTests = expectations.map(expectation => { | ||
| if (expectation.networkRequests && concurrency > 1) { |
There was a problem hiding this comment.
I think we still want to be able to run in parallel locally, at least. Maybe move this check to pruneExpectations() in report-assert.js and remove expectation.networkRequests if !process.env.CI && concurrency > 1 (to enforce doing the extra checks in CI), otherwise throw this error?
There was a problem hiding this comment.
Is there a way to know concurrency in pruneExpectations?
There was a problem hiding this comment.
no, it would have to be passed down in reportOptions (either directly or as a shouldTestNetworkRequests or whatever)
| export type ExpectedRunnerResult = { | ||
| lhr: ExpectedLHR, | ||
| artifacts?: Partial<Record<keyof LH.Artifacts, any>> | ||
| networkRequests?: any; |
There was a problem hiding this comment.
the big question here...what do we actually want to assert about these? :)
There was a problem hiding this comment.
From the issue, it sounds like the best thing to assert is the number of requests.
If we just did networkRequestCount or something, there would be no way to add a _minChromiumVersion for specific network request assertions.
There was a problem hiding this comment.
ah, got it. Maybe networkRequests?: Array<unknown> then to make it clearer but we won't have to bother typing the requests saved from static-server?
Or could do networkRequests?: {length: number} if we wanted to be explicit about what we expect from test authors right now.
Both options would be forward compatible if for some reason we wanted to move to specific object shapes later (networkRequest?: Array<{url: string}> or whatever).
brendankenny
left a comment
There was a problem hiding this comment.
Nice, this looks good!
I think the only open question is if smokerider wants to support networkRequest assertions, since this will fail the tests there as it is now, so it will likely need work either way after this lands and is rolled.
| /** A function that runs Lighthouse with the given options. Defaults to running Lighthouse via the CLI. */ | ||
| lighthouseRunner?: LighthouseRunner; | ||
| /** A function that gets a list of URLs requested to the server since the last fetch. */ | ||
| takeNetworkRequestUrls?: () => string[]; |
There was a problem hiding this comment.
Does this need to be required? It's different than the other options, but if it's not provided then the perf smoke tests will always fail.
Another option would be to prune networkRequests if no takeNetworkRequestUrls is provided, but that is slightly more prone to accidentally not passing it in and the tests silently accepting that and passing anyways
A big question would be if @connorjclark feels like getting the network requests is doable with smokerider. If yes then required seems good. If not, we could go either way (e.g. it could still be required but smokerider could pass in a no-op function and prune out the networkRequests itself in the lib.js frontend).
There was a problem hiding this comment.
currently for smokerider the static server is run in a separate process, so takeNetworkRequestUrls wouldn't work. looking at the code again I see no reason for it being a different process, so should be possible to make this work.
There was a problem hiding this comment.
I've made it required for now.
| requestCountAssertion.push(makeComparison( | ||
| 'Request count', | ||
| server.getRequestUrls(urlPath), | ||
| expected.networkRequests |
There was a problem hiding this comment.
I added some expectations to perf.
dbw might be one of the best to add expectations to since the page tries to be bad at everything and the smoke test runs the full default config. It's also just the one smoke test, so shouldn't be much work to add.
| requestCountAssertion.push(makeComparison( | ||
| 'Request count', | ||
| server.getRequestUrls(urlPath), | ||
| expected.networkRequests |
| const expectations = [ | ||
| { | ||
| networkRequests: { | ||
| length: 59, |
There was a problem hiding this comment.
Does this seem high? The DT panel only shows 25 requests made during the page load.
There was a problem hiding this comment.
The higher number is coming from the three passes, but it does seem kind of high. From
defaultPass: Lighthouse shows 26 network requests
- 2 are
blobandfilesystem(shouldn't be hitting static-server) - 1 is
ajax.googleapis.comfor jquery (shouldn't be hitting static-server) - 23 are for
http://localhost:10200/
(the now poorly named) offlinePass: Lighthouse shows 25 network requests
blob,filesystem, andajax.googleapis.comagain- 22 are for
http://localhost:10200/(1 less because favicon isn't requested on this load)
redirectPass: Lighthouse shows 25 network requests
blobandajax.googleapis.comagain (but filesystem is blocked byblockedUrlPatternsbecause it's a png)- 23 are for
http://localhost:10200/, but only 6 made it through to the server (the rest blocked byblockedUrlPatterns)
That's only 51 by my count though :/
Details
defaultPass: [
{url: "http://localhost:10200/favicon.ico", statusCode: 404},
{url: "http://localhost:10200/dobetterweb/unknown404.css?delay=200", statusCode: 404},
{url: "http://localhost:10200/dobetterweb/third_party/aggressive-promise-polyfill.js", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-rotating.gif", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr3", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr2", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar2", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar1", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000", statusCode: 404},
{url: "http://localhost:10200/dobetterweb/empty.css", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/empty_module.js?delay=500", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.js", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.html", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.html", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?scriptActivated&delay=200", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&async=true", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=100", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_disabled.css?delay=200&isdisabled", statusCode: 200},
{url: "http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js", statusCode: 200},
{url: "filesystem:http://localhost:10200/temporary/empty-0.6220707096123763.png", statusCode: 200},
{url: "blob:http://localhost:10200/a932e885-e346-4e8a-9e56-c3e37c4c427e", statusCode: 200
}
],
offlinePass: [
{url: "http://localhost:10200/dobetterweb/unknown404.css?delay=200", statusCode: 404},
{url: "http://localhost:10200/dobetterweb/third_party/aggressive-promise-polyfill.js", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-rotating.gif", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr3", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr2", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar2", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar1", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000", statusCode: 404},
{url: "http://localhost:10200/dobetterweb/empty.css", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/empty_module.js?delay=500", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.js", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.html", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.html", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?scriptActivated&delay=200", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&async=true", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=100", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_disabled.css?delay=200&isdisabled", statusCode: 200},
{url: "http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js", statusCode: 200},
{url: "http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js", statusCode: 200},
{url: "filesystem:http://localhost:10200/temporary/empty-0.6121657698517076.png", statusCode: 200},
{url: "blob:http://localhost:10200/fa7c68da-ce56-48ea-aaec-31b86bbf2c17", statusCode: 200
}
],
redirectPass: [
{url: "http://localhost:10200/dobetterweb/unknown404.css?delay=200", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/third_party/aggressive-promise-polyfill.js", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/lighthouse-rotating.gif", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr3", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr2", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar2", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar1", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000", statusCode: 404},
{url: "http://localhost:10200/dobetterweb/empty.css", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/empty_module.js?delay=500", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.js", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.html", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.html", statusCode: 200},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?scriptActivated&delay=200", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true", statusCode: -1},
{url: "http://localhost:10200/dobetterweb/dbw_tester.css?delay=100", statusCode: -1},
{url: "http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js", statusCode: 200},
{url: "blob:http://localhost:10200/b653a999-daa6-4573-b7f1-e97e2802ab63", statusCode: 200
}
]
There was a problem hiding this comment.
ok, comparing to what static-server gives us:
Details
[
'/dobetterweb/dbw_tester.html',
'/dobetterweb/dbw_tester.css?delay=2000&async=true',
'/dobetterweb/dbw_tester.css?delay=100',
'/dobetterweb/unknown404.css?delay=200',
'/dobetterweb/dbw_tester.css?delay=2200',
'/dobetterweb/dbw_disabled.css?delay=200&isdisabled',
'/dobetterweb/dbw_tester.css?delay=3000&capped',
'/dobetterweb/dbw_tester.css?delay=3000&async=true',
'/dobetterweb/dbw_tester.js',
'/dobetterweb/empty_module.js?delay=500',
'/dobetterweb/fcp-delayer.js?delay=5000',
'/dobetterweb/clock.appcache',
'/dobetterweb/third_party/aggressive-promise-polyfill.js',
'/dobetterweb/lighthouse-480x318.jpg?iar1',
'/dobetterweb/lighthouse-480x318.jpg?iar2',
'/dobetterweb/lighthouse-480x318.jpg?isr1',
'/dobetterweb/lighthouse-480x318.jpg?isr2',
'/dobetterweb/lighthouse-480x318.jpg?isr3',
'/dobetterweb/lighthouse-480x318.jpg',
'/dobetterweb/lighthouse-rotating.gif',
'/dobetterweb/empty.css',
'/dobetterweb/dbw_tester.css?scriptActivated&delay=200',
'/dobetterweb/dbw_tester.html',
'/dobetterweb/dbw_tester.css?delay=100',
'/dobetterweb/unknown404.css?delay=200',
'/dobetterweb/dbw_tester.css?delay=2200',
'/dobetterweb/dbw_tester.css?delay=3000&capped',
'/dobetterweb/dbw_tester.css?delay=2000&async=true',
'/dobetterweb/dbw_tester.css?scriptActivated&delay=200',
'/dobetterweb/dbw_tester.html',
'/dobetterweb/dbw_tester.css?delay=2000&async=true',
'/dobetterweb/dbw_tester.css?delay=100',
'/dobetterweb/unknown404.css?delay=200',
'/dobetterweb/dbw_tester.css?delay=2200',
'/dobetterweb/dbw_disabled.css?delay=200&isdisabled',
'/dobetterweb/dbw_tester.css?delay=3000&capped',
'/dobetterweb/dbw_tester.css?delay=3000&async=true',
'/dobetterweb/dbw_tester.js',
'/dobetterweb/empty_module.js?delay=500',
'/dobetterweb/fcp-delayer.js?delay=5000',
'/dobetterweb/clock.appcache',
'/dobetterweb/third_party/aggressive-promise-polyfill.js',
'/dobetterweb/lighthouse-480x318.jpg?iar1',
'/dobetterweb/lighthouse-480x318.jpg?iar2',
'/dobetterweb/lighthouse-480x318.jpg?isr1',
'/dobetterweb/lighthouse-480x318.jpg?isr2',
'/dobetterweb/lighthouse-480x318.jpg?isr3',
'/dobetterweb/lighthouse-480x318.jpg',
'/dobetterweb/lighthouse-rotating.gif',
'/dobetterweb/empty.css',
'/dobetterweb/dbw_tester.css?scriptActivated&delay=200',
'/dobetterweb/dbw_tester.html',
'/dobetterweb/dbw_tester.html',
'/dobetterweb/dbw_tester.js',
'/dobetterweb/empty_module.js?delay=500',
'/dobetterweb/fcp-delayer.js?delay=5000',
'/dobetterweb/clock.appcache',
'/dobetterweb/third_party/aggressive-promise-polyfill.js',
'/dobetterweb/dbw_tester.html'
]
favicon apparently doesn't count for anything, but /dobetterweb/clock.appcache does (for all three passes). I forgot that was in that page, kind of annoying that it doesn't show up in the network requests (but it'll be removed from Chrome soon enough, so whatever).
That's 53, and the remaining 6 requests are re-requests of all the css files:
'/dobetterweb/dbw_tester.css?delay=100',
'/dobetterweb/unknown404.css?delay=200',
'/dobetterweb/dbw_tester.css?delay=2200',
'/dobetterweb/dbw_tester.css?delay=3000&capped',
'/dobetterweb/dbw_tester.css?delay=2000&async=true',
'/dobetterweb/dbw_tester.css?scriptActivated&delay=200',
They appear to be happening in defaultPass, but I verified there's no Network.requestWillBeSent events in any of the passes that would account for these requests, so either they're happening outside of pass() (so not being picked up by NetworkRecorder) or they're some out-of-band request (like the appcache manifest is) that DevTools doesn't get info on via Network events
There was a problem hiding this comment.
OK, can confirm that those six requests are happening at the end of defaultPass (or at least not during the other passes...deleted the other passes and those six requests were still recorded by static-server).
It's possible this is one of the bugs this assertion was being added to prevent :) I'll keep looking but if anyone else has any ideas as to the source of these, please let us know.
There was a problem hiding this comment.
Unfortunately this isn't a solution or an answer to the mystery. It looks like a similar problem is occurring on http://localhost:10200/preload.html. The requests are:
"/preload.html",
"/perf/level-2.js?warning&delay=500",
"/perf/preload_style.css",
"/perf/preload_tester.js",
"/perf/lobster-v20-latin-regular.woff2",
"/perf/level-2.js?delay=500",
"/perf/level-3.js",
"/perf/level-2.js?warning&delay=500",
"/perf/preload_style.css"
/perf/preload_style.css is fetched twice.
There was a problem hiding this comment.
hmm, maybe a gatherer?
There was a problem hiding this comment.
simply opening devtools on the page (but only the first time) makes the extra requests to CSS files.
or at least not during the other passes
Try any pass in isolation - I expect only the first is triggering the issue.
There was a problem hiding this comment.
Looks like it's CSS.enable in particular. If you don't run the gatherers CSSUsage, ImageElements, and FontSize you don't get those extra requests.
(I assume opening DevTools calls CSS.enable by default)
There was a problem hiding this comment.
To close this up, these extra requests are intentional. Chrome is aggressive with evicting raw stylesheet contents from memory, so DevTools reacquires them when CSS.enable is called. It hits the disk cache first (original change: https://codereview.chromium.org/24151005), but we don't set cache headers on our requests, so the test stylesheets aren't in the disk cache and are re-fetched.
A comment is probably sufficient to document this, but I guess we'll see how annoying this number is to maintain in the future :) Does the next person touching dbw_tester have to go through the same logging-URL exercise to make sure the new number is reasonable?
Verifies
networkRequestsalongsidelhrandartifacts.Groups requests by socket and then remaps based on the first URL to get the requested URLs for each smoke test. Only surfaces requests made to the smoke server at
localhost:10200.Closes #11506