1405243 - Migrate browser_webconsole_bug_1006027_message_timestamps_incorrect.js to a server test

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

This test should be on the server to make sure we always send the correct timestamp in console log packet

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1405243 - Migrate browser_webconsole_bug_1006027_message_timestamps_incorrect.js to a server test; .

The test was testing that the 3 messages we display whe
evaluating a console.log (text of the input, evaluation result,
consoleAPI message) all had a timestamp in a 2000ms range.
This wasn't really what it should have been about, since the
original bugfix (for Bug 1405243), was only in Console.cpp (and
the fix is still up there).
So here we only assert that the timestamp in the ConsoleAPI packet
is at most 1000ms more than when the log was requested.

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

Comment 2

[Brian Grinstead [:bgrins]]

I'm seeing this error in mozreview: "You don't have access to this review request. This review request is private. You must be a requested reviewer, either directly or on a requested group, and have permission to access the repository in order to view this review request."

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8960201 [details]
Bug 1405243 - Migrate browser_webconsole_bug_1006027_message_timestamps_incorrect.js to a server test; .

https://reviewboard.mozilla.org/r/228956/#review234696

::: commit-message-0b997:3
(Diff revision 1)
> +Bug 1405243 - Migrate browser_webconsole_bug_1006027_message_timestamps_incorrect.js to a server test; r=bgrins.
> +
> +The test was testing that the 3 messages we display whe

Typo "when"

::: devtools/shared/webconsole/test/test_console_timestamp.html:32
(Diff revision 1)
> +    const clientLogTimestamp = Date.now();
> +    const packet = await consoleAPICall(debuggerClient,
> +      () => top.console.log("test"));
> +
> +    const packetTimestamp = packet.message.timeStamp;
> +    const LOG_PROPAGATION_DELTA_MS = 1000;

I hope this isn't prone to intermittents, but you never know on debug builds. We could consider going back to 2000ms as the current test does, or instead taking another Date.now() measurement once packet has resolved and theoretically the timeStamp on the packet should be in between this. Up to you.

::: devtools/shared/webconsole/test/test_console_timestamp.html:44
(Diff revision 1)
> +  await closeDebugger(state);
> +  SimpleTest.finish();
> +};
> +
> +
> +function consoleAPICall(debuggerClient, consoleCall) {

Can you pull this into a helper function in common.js? Looks like we use this in two other places as well: https://searchfox.org/mozilla-central/search?q=addOneTimeListener%28%22consoleAPICall%22%29&path=

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8960201 [details]
Bug 1405243 - Migrate browser_webconsole_bug_1006027_message_timestamps_incorrect.js to a server test; .

https://reviewboard.mozilla.org/r/228956/#review234696

> I hope this isn't prone to intermittents, but you never know on debug builds. We could consider going back to 2000ms as the current test does, or instead taking another Date.now() measurement once packet has resolved and theoretically the timeStamp on the packet should be in between this. Up to you.

oh yeah, sounds much more better

> Can you pull this into a helper function in common.js? Looks like we use this in two other places as well: https://searchfox.org/mozilla-central/search?q=addOneTimeListener%28%22consoleAPICall%22%29&path=

yes

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1405243 - Extract consoleAPICall helper function in common.js; .

The function was used through multiple tests so it makes sense
to have it in common.js.
We take this as an opportunity to unify the style of the tests, and
to make closeDebugger return a Promise.

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8960201 [details]
Bug 1405243 - Migrate browser_webconsole_bug_1006027_message_timestamps_incorrect.js to a server test; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228956/diff/1-2/

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8960247 [details]
Bug 1405243 - Extract consoleAPICall helper function in common.js; .

https://reviewboard.mozilla.org/r/229000/#review234758

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3610b83bb375
Migrate browser_webconsole_bug_1006027_message_timestamps_incorrect.js to a server test; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/d4b5b83166e2
Extract consoleAPICall helper function in common.js; r=bgrins.

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3610b83bb375
https://hg.mozilla.org/mozilla-central/rev/d4b5b83166e2