1460590 - Figure out why server timing parsing tests are failing

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

Bug 1460226 makes the server timing tests run over HTTPS.
Running `./mach wpt testing/web-platform/tests/server-timing/server_timing_header-parsing.https.html` shows 6 failures. We should figure out if we need to change the parsing behaviour or if it's the tests that need fixing.
See server_timing_header-parsing.https.html.ini for the failures.

Comment 1

[u408661]

Attachment: Bug 1460590 part 1 - Fix our parsing of Server-Timing.

We were erroneously looking for the first reasonably-valued
server-timing-param for each name. However, that's not how it works. We
should really be looking for the first server-timing-param regardless
of how reasonable its value is.

Review commit: https://reviewboard.mozilla.org/r/248582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248582/

Comment 2

[u408661]

Attachment: Bug 1460590 part 2 - Fix buggy tests.

Test 67.js was expecting us to only parse out one server-timing-metric,
and ignore "junk" after. But, according to the spec, we are only to
ignore junk up to the point where we see either (1) another metric, or
(2) a server-timing-param. The header sent has a param (dur=123.4), and
we were parsing that correctly.

Test 68.js was similar, but this time there was another metric (comma-
separated) in the header, which we correctly parsed. The test expected
us to ignore it as "junk".

Review commit: https://reviewboard.mozilla.org/r/248584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248584/

Comment 3

[u408661]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d4769b0749623f32b117ae890d9f1207a12dfa1

Comment 4

[James Graham [:jgraham]]

Comment on attachment 8983085 [details]
Bug 1460590 part 2 - Fix buggy tests.

https://reviewboard.mozilla.org/r/248584/#review255100

OK, but I haven't reviewed the spec to check that your interpretation is correct.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8983084 [details]
Bug 1460590 part 1 - Fix our parsing of Server-Timing.

https://reviewboard.mozilla.org/r/248582/#review255138

Thanks!

Comment 6

[u408661]

Valentin - given comment 4, would you mind giving the spec a double-check for me before I land, to ensure I'm not missing something that would make my interpretation wrong? Thanks.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Agreed. I reviewed the spec part regarding header parsing [1] and it is consistent with the changes to 67.js and 68.js


[1] https://w3c.github.io/server-timing/#-dfn-server-timing-header-parsing-algorithm-dfn-

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94b826fc4ea4
part 1 - Fix our parsing of Server-Timing. r=valentin
https://hg.mozilla.org/integration/autoland/rev/51202d93e2e3
part 2 - Fix buggy tests. r=jgraham

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/94b826fc4ea4
https://hg.mozilla.org/mozilla-central/rev/51202d93e2e3

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11435 for changes under testing/web-platform/tests

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged