Skip to content
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 request count assertion #12325

Merged
merged 10 commits into from
Apr 15, 2021
Merged

Conversation

adamraine
Copy link
Member

Verifies networkRequests alongside lhr and artifacts.

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

@adamraine adamraine requested a review from a team as a code owner April 2, 2021 16:37
@adamraine adamraine requested review from paulirish and removed request for a team April 2, 2021 16:37
@google-cla google-cla bot added the cla: yes label Apr 2, 2021
lighthouse-cli/test/fixtures/static-server.js Outdated Show resolved Hide resolved
lighthouse-cli/test/fixtures/static-server.js Outdated Show resolved Hide resolved
requestCountAssertion.push(makeComparison(
'Request count',
server.getRequestUrls(urlPath),
expected.networkRequests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it'd be good to exercise the actual assertions within the PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some expectations to perf.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@brendankenny brendankenny Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@brendankenny brendankenny Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

// If defn is set to `runSerially`, we'll run its tests in succession, not parallel.
const concurrency = testDefn.runSerially ? 1 : jobs;

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.

Copy link
Member

@brendankenny brendankenny Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just runSerially: true it 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added dbw.

@adamraine
Copy link
Member Author

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?

@paulirish
Copy link
Member

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.

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.

Any ideas on how to proceed?

One idea.. cookies. But tbh i don't like it all.
Another idea.. the Referrer header. But I also think we'll be fighting against some privacy things that I don't think its worth dealing with.

If we restrict these assertions to only work on runSerially: true smoketests, that will simplify things quite a bit. We can have resetRequestCount() in the server. And.. keeping things really stupid, maybe use a &reset_count param on the document URL that'd call that. (Seems a bit easier than another heuristic to determine top-level document requests.)

@adamraine
Copy link
Member Author

If we restrict these assertions to only work on runSerially: true smoketests

Ok, I think this is the best option to pursue for now.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks cool to me.

if (expected.networkRequests) {
requestCountAssertion.push(makeComparison(
'Requests',
server.getRequestUrls(),
Copy link
Member

@brendankenny brendankenny Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

lighthouse-cli/test/smokehouse/smokehouse.js Outdated Show resolved Hide resolved
isDebug,
}));
const individualTests = expectations.map(expectation => {
if (expectation.networkRequests && concurrency > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to know concurrency in pruneExpectations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it would have to be passed down in reportOptions (either directly or as a shouldTestNetworkRequests or whatever)

@@ -19,6 +19,7 @@
export type ExpectedRunnerResult = {
lhr: ExpectedLHR,
artifacts?: Partial<Record<keyof LH.Artifacts, any>>
networkRequests?: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the big question here...what do we actually want to assert about these? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lighthouse-cli/test/smokehouse/smokehouse.js Outdated Show resolved Hide resolved
lighthouse-cli/test/fixtures/static-server.js Show resolved Hide resolved
@@ -41,6 +42,8 @@
retries?: number;
/** 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[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it required for now.

requestCountAssertion.push(makeComparison(
'Request count',
server.getRequestUrls(urlPath),
expected.networkRequests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added dbw.

@@ -11,6 +11,9 @@
*/
const expectations = [
{
networkRequests: {
length: 59,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this seem high? The DT panel only shows 25 requests made during the page load.

Copy link
Member

@brendankenny brendankenny Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 blob and filesystem (shouldn't be hitting static-server)
  • 1 is ajax.googleapis.com for 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, and ajax.googleapis.com again
  • 22 are for http://localhost:10200/ (1 less because favicon isn't requested on this load)

redirectPass: Lighthouse shows 25 network requests

  • blob and ajax.googleapis.com again (but filesystem is blocked by blockedUrlPatterns because it's a png)
  • 23 are for http://localhost:10200/, but only 6 made it through to the server (the rest blocked by blockedUrlPatterns)

That's only 51 by my count though :/

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
  }
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, comparing to what static-server gives us:

[
  '/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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, maybe a gatherer?

Copy link
Collaborator

@connorjclark connorjclark Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simply opening devtools on the page (but only the first time) makes the extra requests to CSS files.

image

or at least not during the other passes

Try any pass in isolation - I expect only the first is triggering the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add server request count assertions to smoke tests
5 participants