move computed artifacts dependent on networkRecords to devtoolsLog #2467
Conversation
| artifacts.requestNetworkRecords(devtoolsLogs), | ||
| artifacts.requestNetworkThroughput(devtoolsLogs) | ||
| ]).then(([networkRecords, networkThroughput]) => { | ||
| let totalBytes = 0; |
There was a problem hiding this comment.
the changes here (and in critical-request-chains.js) are just indentation (and, sadly, just reversing the indenting #2062 did :P)
| @@ -26,8 +26,6 @@ describe('NetworkThroughput', () => { | |||
| } | |||
|
|
|||
| it('should return Infinity for no/missing records', () => { | |||
| assert.equal(compute(undefined), Infinity); | |||
| assert.equal(compute(null), Infinity); | |||
There was a problem hiding this comment.
since the network records will now always come from requestNetworkRecords, there will always be some array, so these aren't necessary anymore. I may be missing a failure mode, though, because I don't remember how these were possible in the first place. Please clue me in if I am missing something :)
There was a problem hiding this comment.
I don't think I expected them to be null, probably just overly defensive
| const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS]; | ||
| return artifacts.requestNetworkRecords(devtoolsLog) | ||
| .then(networkRecords => this.audit_(artifacts, networkRecords)) | ||
| .then(result => { |
There was a problem hiding this comment.
this restructuring seems unnecessary just trying to remove nesting?
There was a problem hiding this comment.
just trying to remove nesting?
yeah, and it gets rid of the Promise.resolve. I tried a few different permutations and this was a less awkward version than others, but I can revert to
return artifacts.requestNetworkRecords(devtoolsLogs).then(networkRecords => {
return artifacts.requestNetworkThroughput(devtoolsLogs).then(networkThroughput =>
Promise.resolve(this.audit_(artifacts, networkRecords)).then(result =>
this.createAuditResult(result, networkThroughput)
)
);
});if you want.
There was a problem hiding this comment.
eh not a big deal, this is fine
There was a problem hiding this comment.
awkwardness like this due to needing to hold onto variables will be a lot better once we can use async/await
| const assert = require('assert'); | ||
| const Gatherer = new GathererClass(); | ||
|
|
||
| const Runner = require('../../../runner.js'); |
There was a problem hiding this comment.
well that's annoying, can't we just pass an obj like {requestNetworkRecords: () =>...} to compute_ instead?
There was a problem hiding this comment.
these tests are already cheating by assuming the exact properties of network requests the gatherer will need, so that's probably fine.
There was a problem hiding this comment.
done. Also added one test that does the end to end devtoolsLog -> CRC extraction
| @@ -26,8 +26,6 @@ describe('NetworkThroughput', () => { | |||
| } | |||
|
|
|||
| it('should return Infinity for no/missing records', () => { | |||
| assert.equal(compute(undefined), Infinity); | |||
| assert.equal(compute(null), Infinity); | |||
There was a problem hiding this comment.
I don't think I expected them to be null, probably just overly defensive
| @@ -7,15 +7,17 @@ | |||
|
|
|||
| /* eslint-env mocha */ | |||
|
|
|||
| const GathererClass = require('../../../gather/computed/pushed-requests'); | |||
| const Runner = require('../../../runner.js'); | |||
There was a problem hiding this comment.
I can update this one too, but I don't even know why we have this gatherer and really am tempted to just delete it :)
There was a problem hiding this comment.
ok, so I'll wait until Paul gets back to delete, but also don't care enough to update :)
I think it's fine with instatiateComputedArtifacts, and it's annoying enough to recreate that network record in a devtoolsLog (that site doesn't appear to be using http2 push anymore?) that mocking requestNetworkRecords doesn't seem like the worrrrst thing.
wdyt?
followup to #2062 and #2133 (specifically #2062 (comment))
really just a convenience thing. Since computed artifacts can request other computed artifacts, callers of e.g. criticalRequestChains shouldn't have to construct networkRecords, CRC can just do it itself.