oops, forgot to remove.

Note that this code has moved. I tried to explain in the comment elsewhere.

oops, forgot to remove

remove now?

Kompitabel korleis type i dag

adding the extra new properties could possibly wait until all the other audits are switched over to using them (where appropriate). That would keep those changes separate from the renames.

Kom bedre å bare ta det litt av Kombef er litt for tynn Henrik

Easier just to s/startTime/networkRequestTime/g, s/endTime/networkEndTime/g and s/responseReceivedTime/responseHeadersEndTime/g? Seems unnecessary to have the codebase use both (and necessitates the network-records-to-devtools-log.js workaround) when there's very little review work for just a property rename.

I proposed that we do it this way.. i would prefer it cuz every since place where where we use startTime we should be very deliberate about which of the new timings we want.

And when I replicated the renames a bit ago, I caught a few cases that didn't make sense to me. :)

The big reason for this approach is that this PR is just 7 files (less, if I take your suggestion to defer network requests audit changes)- but it would be dozens of files mixed in with the important changes here if done all at once.

The few lines in records to DevTools log is just encoding the framework we'd mostly use during the conversion process (next PR).

I was probably too focused on an exact approach in my suggestion :) I think we're all broadly going for the same thing, my main issue was just anticipating a PR where some startTimes get changed to networkRequestTime and some get changed to rendererStartTime (and similar for endTime) and a reviewer will have to read each line carefully to double check which one was used.

I was attempting to suggest

• a PR that only updates startTime to networkRequestTime (and endTime to networkEndTime) so no actual timing/test values changed
• a PR that changes some of those networkRequestTimes to rendererStartTime (and networkEndTime to rendererEndTime), which would involve some timing/test value changes. That way the changes we want to be most deliberate about are highlighted (any property being changed is being switched to a new timing value).

If intermediate PRs before that point help break it up nicely, SGTM, I won't object to that :)

a PR that only updates startTime to networkRequestTime (and endTime to networkEndTime) so no actual timing/test values changed

Doing this next makes sense for the reasons you highlighted. Will do that. Hopefully it'll be the fastest on these PRs!

i'm not sure about this * 1000, given that rendererstartime should be in ms.

but... this is a problem for @brendankenny as our debugData consumer :)

i'm not sure about this * 1000, given that rendererstartime should be in ms.

Yeah, I guess it's debatable what it should use. My original thinking was it should use microseconds because you compare it against observedNavigationStartTs to get the offset between CPU task timestamps and network times:

(CAST(JSON_VALUE(lighthouse, '$.audits."network-requests".details.debugData.networkStartTimeTs') AS FLOAT64) - CAST(JSON_VALUE(lighthouse, '$.audits.metrics.details.items[0].observedNavigationStartTs') AS FLOAT64)) / 1000 AS network_ts_offset

I put the Ts at the end of networkStartTimeTs to try to communicate that, but it's also a good argument that it's simpler if this audit only had values in ms 🤷

I proposed that we do it this way.. i would prefer it cuz every since place where where we use startTime we should be very deliberate about which of the new timings we want.

And when I replicated the renames a bit ago, I caught a few cases that didn't make sense to me. :)

Does this mean the test should be using rendererEndTime instead of networkEndTime or in addition to it? Seems like it should be in addition to, which is really a TODO to add tests when we add rendererEndTime support?

test looks good, can remove/update the comment?

add networkRequestTime to these as well?

done.

but why is the second item here considered finished?

timing defaults to undefined, which is what audits depending on it check before accessing (e.g. uses-rel-preconnect), and the falsy check below will currently always be true

ugh, the problem here is that networkRequestTime isn't properly set when timing is missing, failing 8 tests in network-request-test.js.

For example, take update fetch stats for Lightrider test. the startTime/networkRequestTime value is now 50 instead of 0, because I wanted some separation between the first two timing properties. Without a timing object, such a distinction can never be made. So to roundtrip correctly I had to create the object here.

Should I 1) fix the tests to provide the relevant timing object to make the values come out right or 2) do a check in this records->log conversion if these properties differ, and only create a timing object magically...to reduce some test boilerplate. Leaning towards 1.

The goal of network-records-to-devtools-log is to take whatever super-partial network record you give it and make a plausible network record out of it, and if you want a more specific value somewhere in the generated record, you just have to pass it in. So we should go for 2 if possible. Having rendererStartTime === networkRequestTime seems ok if only one is provided?

maybe it would be easier to always create a timing object now?

fwiw for current PR code, willNeedTimingObject is always false for uses-rel-preconnect ... and there are zero tests in that audit that exercise a missing timing object. Is this a valid scenario?

bump for the above question

fwiw for current PR code, willNeedTimingObject is always false for uses-rel-preconnect ... and there are zero tests in that audit that exercise a missing timing object. Is this a valid scenario?

Maybe just for failed requests? I'm not sure if it's important, it's possible it's just inherited from super old SDK code, or it could handle some important case we're not thinking of :)

don't these both need to be set if one of them is going to be?

Yeah, but the idea here was to never override a provided timing object, in case a test defined one (even partially).

Maybe instead this should check if both these timing fields are undefined before assigning either?

I thought I could add a precondition here:

if (record.rendererStartTime === undefined ^ record.networkEndTime === undefined) {
        // eslint-disable-next-line max-len
        throw new Error('expected test record to define rendererStartTime and networkEndTime together, but never just one');
      }

(I tried asserting on either being missing but way more failed than I wanted to check)

But 10 tests threw errors because they mocked network records and only defined startTime (or endTime). Should I update those tests so we can ensure more realistic values here?

actually all are from uses-rel-preload, and all just set startTime leaving endTime undefined.

it'd be easy to set endTime to something in those tests (-1, I guess that is the implicit default getting used?)

or maybe assign endTime -1 if startTime is defined and endTime is missing on a test record object?

it's fine to update the tests, but we do want some solution in here for future test writers, either picking a reasonable value or cause some failure (e.g. in the roundtripping) so the test author knows something is wrong

I thought this would work:

if (record.rendererStartTime === undefined) {
        record.rendererStartTime = 0;
      }
      if (record.networkEndTime === undefined) {
        record.networkEndTime = -1;
      }

But had complications getting it to work w/ the redirectResponseTimestamp kludge used elsewhere. I'm getting a bit off track here, so gonna forget about normalizing the test data for now.

I can remove this object literal check and the one below and tests still all pass. I believe only the 'should roundtrip a real devtools log properly' test actual passes in real NetworkRequest instances. It seems wrong to modify those though, hence the checks here.

ideally this is just needed for the property name transition, so this seems fine as a temporary measure

test looks good, can remove/update the comment?

oops, forgot to remove

remove now?

seems like this could still be up in getResponseReceivedEvent rather than modifying the record. Could move back after the startTime/endTime gets removed

Ah, I recall why I did it this way. See the test failure for yarn mocha gatherers/inspector-issues:

      "sessionId": undefined,
      "statusCode": -1,
-     "timing": undefined,
+     "timing": Object {
+       "receiveHeadersEnd": 0,
+       "requestTime": -0.001,
+     },
      "transferSize": 0,
      "url": "https://example.com",
    },
  ]
      at networkRecordsToDevtoolsLog (file:///Users/cjamcl/src/lighthouse/core/test/network-records-to-devtools-log.js:295:40)
      at Context.<anonymous> (file:///Users/cjamcl/src/lighthouse/core/test/gather/gatherers/inspector-issues-test.js:329:19)
      at processImmediate (node:internal/timers:466:21)

Since timing object presence is now being automatically forced, the roundtrip assertion trips up unless we do the modification on the (cloned) test record.

So uh.... should I put it back in networkRecordsToDevtoolsLog?

Part of it is inspector-issues-test is weird: it's Object.assigning to a real NetworkRequest, then passing it through networkRecordsToDevtoolsLog (it could have just used the partial passed through networkRecordsToDevtoolsLog, but it looks like those lines came a long time after the initial mockRequest method was written).

But even fixing that, there's a deeper problem and I don't think we can get away with optionally adding timing anymore. If you add

diff --git a/core/test/network-records-to-devtools-log-test.js b/core/test/network-records-to-devtools-log-test.js
index d114d483b..dd176e057 100644
--- a/core/test/network-records-to-devtools-log-test.js
+++ b/core/test/network-records-to-devtools-log-test.js
@@ -20,6 +20,11 @@ describe('networkRecordsToDevtoolsLog', () => {
       {method: 'Network.dataReceived'},
       {method: 'Network.loadingFinished'},
     ]);
+
+    const roundTripRecords = NetworkRecorder.recordsFromLogs(log);
+    const logNo2 = networkRecordsToDevtoolsLog(roundTripRecords);
+    const roundTripRecordsNo2 = NetworkRecorder.recordsFromLogs(logNo2);
+    expect(roundTripRecordsNo2).toMatchObject(roundTripRecords);
   });
 
   it('should set resource and transfer sizes', () => {

and run yarn mocha network-records-to-devtools-log this fails on this branch but passes on main. The first round-trip check (inside networkRecordsToDevtoolsLog) passes, but running the output through it again it fails on the same timing undefined vs Object part (the first time through it doesn't add a timing object, but the second time it does)

ideally this is just needed for the property name transition, so this seems fine as a temporary measure

it's fine to update the tests, but we do want some solution in here for future test writers, either picking a reasonable value or cause some failure (e.g. in the roundtripping) so the test author knows something is wrong

fwiw for current PR code, willNeedTimingObject is always false for uses-rel-preconnect ... and there are zero tests in that audit that exercise a missing timing object. Is this a valid scenario?

Maybe just for failed requests? I'm not sure if it's important, it's possible it's just inherited from super old SDK code, or it could handle some important case we're not thinking of :)