Closed Bug 1473332 Opened 4 years ago Closed 4 years ago

Add DAMP test for Autocomplete popup

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [boogaloo-mvp])

Attachments

(1 file)

The test should measure how much time it takes to open, filter and close the popup
Priority: -- → P2
Whiteboard: [boogaloo-mvp]
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8990665 [details]
Bug 1473332 - Add a DAMP test for console autocomplete; .

https://reviewboard.mozilla.org/r/255748/#review262474

It looks good to me, but I would like to see if you could lower the typical variance of this test.
As-is, it will harly catch regressions under 3%.
The best is to try various settings and push to try.

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:20
(Diff revision 1)
> +const TEST_NAME = "console.autocomplete";
> +
> +
> +module.exports = async function() {
> +  await testSetup(`data:text/html,<meta charset=utf8><script>
> +    const items = Array.from({length: 100}).reduce((res, _, i) => ({

Did you tried other number other than 100?
And/or tried doing more than one autocomplete?

As-is the test is having a noise of 2.9%, while I tried to make it so that most of the tests (especialy the open and reload ones) are under 1%:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=2f89910ad07652e1f99556558c078dd788d1cae1&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=auto&framework=1&selectedTimeRange=172800

Tweaking the test to be more intense sometimes lower its noise. Otherwise running in more than once helps.

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:23
(Diff revision 1)
> +module.exports = async function() {
> +  await testSetup(`data:text/html,<meta charset=utf8><script>
> +    const items = Array.from({length: 100}).reduce((res, _, i) => ({
> +      ...res,
> +      ["item" + i.toString().padStart(2, "0")]: i
> +    }), {});

In term of performance, this code is very bad.
It will:
* call a method 100 times whereas we could use a loop
* create an object 100 times while duplicating the previous object that is big and bigger 100 times!!

Not whithout saying it is criptic when you don't know these two "advanced" methods: Array.from and reduce.

i.e. please use a loop here and look more like C code :)

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:48
(Diff revision 1)
> +  // order to display the popup.
> +  jsterm.complete(jsterm.COMPLETE_HINT_ONLY);
> +  await onPopUpOpened;
> +  test.done();
> +
> +  await closeToolboxAndLog(TEST_NAME, toolbox);

Is there really a value in tracking close?
It doesn't looks like a typical regression to watch.
Attachment #8990665 - Flags: review?(poirot.alex)
Comment on attachment 8990665 [details]
Bug 1473332 - Add a DAMP test for console autocomplete; .

https://reviewboard.mozilla.org/r/255748/#review265756

Thanks Nicolas, it looks good.

Do you have a link to DAMP for this patch?

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/autocomplete.js:45
(Diff revisions 1 - 2)
> +    await showAndHideAutoCompletePopup(jsterm);
> +  }
> +
> +  test.done();
> +
> +  await closeToolbox(TEST_NAME, toolbox);

closeToolbox expects no arguments
Attachment #8990665 - Flags: review?(poirot.alex) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> Here's a try push with the updated test
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&newProject=try&newRevision=a96e52eafe34bbb8fd25d90329f6fea9a95fc0f1&f
> ramework=1&filter=damp&selectedTimeRange=172800

1.26%, it is pretty good noise, thanks for the improvement!
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5865394c3ef1
Add a DAMP test for console autocomplete; r=ochameau.
https://hg.mozilla.org/mozilla-central/rev/5865394c3ef1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1477912
You need to log in before you can comment on or make changes to this bug.