1368431 - Add network monitor tests for searchbox autocomplete

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Tim Nguyen :ntim]

We have a test for the component, but no test for the network monitor feature.

Comment 1

[Ruturaj Vartak]

Attachment: browser_net_filter-autocomplete.js

Tim,

Till the bug#1364093 gets checked-in, I thought you could have a look at this one. I'll send a review request on an actual patch.

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8872967 [details]
browser_net_filter-autocomplete.js


>  expected.map(function (item, i) {
>    is(
>      document
>        .querySelector(`.devtools-autocomplete-listbox .autocomplete-item:nth-child(${i + 1})`)
>        .textContent,
>      item,
>      `${expected[i]} found`
>    );
>  });
Use forEach instead of map.


>add_task(async function () {
>  let { monitor } = await initNetMonitor(FILTERING_URL);
>  let { document, window } = monitor.panelWin;
>
>  info("Starting test... ");
>
>  EventUtils.synthesizeMouseAtCenter(
>    document.querySelector(".devtools-filterinput"), {}, window);
>  // Empty Mouse click should keep autocomplete hidden
>  ok(!document.querySelector(".devtools-autocomplete-popup"),
>    "Autocomplete Popup Created");
>
>  // Typing a char should invoke a autocomplete
>  EventUtils.synthesizeKey("s", {});
>  ok(document.querySelector(".devtools-autocomplete-popup"),
>    "Autocomplete Popup Created");
>  testAutocompleteContents([
>    "scheme:",
>    "set-cookie-domain:",
>    "set-cookie-name:",
>    "set-cookie-value:",
>    "size:",
>    "status-code:",
>  ], document);
>
>  EventUtils.synthesizeKey("c", {});
>  testAutocompleteContents(["scheme:"], document);
>  EventUtils.synthesizeKey("VK_TAB", {});
>  // Tab selection should hide autocomplete
>  ok(!document.querySelector(".devtools-autocomplete-popup"),
>    "Autocomplete Popup Created");
You might want to test the value is scheme: at this point.

>  // Space separated tokens
>  EventUtils.synthesizeKey(" ", {});
>  // Adding just a space should keep popup hidden
>  ok(!document.querySelector(".devtools-autocomplete-popup"),
>    "Autocomplete Popup Created");
>
>  // The last token where autocomplete is availabe shall generate the popup
>  EventUtils.synthesizeKey("p", {});
>  testAutocompleteContents(["protocol:"], document);
>
>  // The new value of the text box should be previousTokens + latest value selected
>  EventUtils.synthesizeKey("VK_RETURN", {});
>  is(document.querySelector(".devtools-filterinput").value,
>    "scheme: protocol:", "Tokenized click generates correct value in input box");
A more realistic scenario would be "scheme:https protocol:"
But perhaps that should be tested after bug 1364096.

Comment 3

[Ruturaj Vartak]

Attachment: fix-1368431-1.patch

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8876052 [details] [diff] [review]
fix-1368431-1.patch

Review of attachment 8876052 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if the test passes :)

::: devtools/client/netmonitor/test/browser_net_filter-autocomplete.js
@@ +46,5 @@
> +  // Tab selection should hide autocomplete
> +  ok(!document.querySelector(".devtools-autocomplete-popup"),
> +    "Autocomplete Popup Hidden");
> +  ok(document.querySelector(".devtools-filterinput"),
> +    "scheme:");

Shouldn't this be : document.querySelector(".devtools-filterinput").value ?

Comment 5

[Ruturaj Vartak]

Attachment: fix-1368431-2.patch

Hey Tim,
Yes it should've been `is` with .value's use. Fixed that. The test is passing :)
Thanks.

PS: You changed your email id?

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8876189 [details] [diff] [review]
fix-1368431-2.patch

Review of attachment 8876189 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me !

Comment 7

[Ruturaj Vartak]

Thanks Tim.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb85e39d0383
Add network monitor tests for searchbox autocomplete. r=ntim

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Sorry, this had to be backed out for too long line at browser_net_filter-autocomplete.js:10:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35bdd25a810319b52365a12da51df0ac2426c6cc

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=eb85e39d0383980f7d698e4bb163c14ef322c137&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=106068132&repo=mozilla-inbound
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/test/browser_net_filter-autocomplete.js:10:1 | Line 10 exceeds the maximum line length of 90. (max-len)

Please fix the issue and submit an updated patch. Thank you.

Comment 10

[Ruturaj Vartak]

Attachment: fix-1368431-3.patch

Updated the line #10.

Strange, the eslinter nor `./mach eslint` didn't show any issues.

Comment 11

[Ruturaj Vartak]

should I assign the checkin-needed flag?

Comment 12

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #11)
> should I assign the checkin-needed flag?

It should be fine to do so once you've got an r+ (which is the case here).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a33646645e
Add network monitor tests for searchbox autocomplete. r=ntim

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54a33646645e