core(network-analyzer): move throughput to NetworkAnalyzer #5900
Conversation
| @@ -1825,53 +1825,43 @@ | |||
| "items": [ | |||
| { | |||
| "url": "http://localhost:10200/zone.js", | |||
| "totalBytes": 71654, | |||
| "totalMs": 409.9376550703614 | |||
There was a problem hiding this comment.
These are technically breaking changes, but the Ms that was being returned was next to useless. We can return the Ms from lantern instead, but they weren't being surfaced so didn't seem worth it.
|
Anyone have strong feelings about this one? |
lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
Outdated
Show resolved
Hide resolved
| @@ -273,6 +273,103 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('#estimateThroughput', () => { | |||
There was a problem hiding this comment.
Do we need any tests for when responseReceivedTime or endTime is -1 or undefined or null etc.
| } | ||
| }); | ||
|
|
||
| return totalBytes * 8 / totalDuration; |
There was a problem hiding this comment.
Just curious, if we only show this in bytes in the end, and all the tests add a * 8, why does this function use bits?
There was a problem hiding this comment.
Historically it was measured in bytes (which is why all the tests are now * 8), but because this PR moves the code into network throughput where everything is measured in bits I wanted to make it consistent. Since I was just doing a straight copy with minimal changes and network records use bytes, I thought it was most straightforward to do it at the end but could also do a totalBytes -> totalBits rename and do it as it sums if you think that's clearer :)
There was a problem hiding this comment.
I think to me it would be clearer if it collected the totalBits and then return that, but then that's a lot more multiplication for very little reason. It's fine like this if the standard is to return bits, I just thought it was funny to calculate the bits then just do bytes * 8 for the tests.
Leftover cleanup from the creation of network analyzer