Closed Bug 1368431 Opened 5 years ago Closed 5 years ago

Add network monitor tests for searchbox autocomplete

Categories

(DevTools :: Netmonitor, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: ntim, Assigned: ruturaj)

References

Details

Attachments

(1 file, 3 obsolete files)

We have a test for the component, but no test for the network monitor feature.
Assignee: nobody → ruturaj
Depends on: 1364093
Attached file browser_net_filter-autocomplete.js (obsolete) —
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.
Flags: needinfo?(ntim.bugs)
Attachment #8872967 - Attachment mime type: application/javascript → text/javascript
Flags: needinfo?(ntim.bugs)
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.
Attachment #8872967 - Flags: feedback+
Attached patch fix-1368431-1.patch (obsolete) — Splinter Review
Attachment #8872967 - Attachment is obsolete: true
Attachment #8876052 - Flags: review?(ntim.bugs)
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 ?
Attachment #8876052 - Flags: review?(tinguyen) → review+
Attached patch fix-1368431-2.patch (obsolete) — Splinter Review
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?
Attachment #8876052 - Attachment is obsolete: true
Attachment #8876189 - Flags: review?(tinguyen)
Comment on attachment 8876189 [details] [diff] [review]
fix-1368431-2.patch

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

Looks good to me !
Attachment #8876189 - Flags: review?(tinguyen) → review+
Keywords: checkin-needed
Thanks Tim.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb85e39d0383
Add network monitor tests for searchbox autocomplete. r=ntim
Keywords: checkin-needed
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.
Flags: needinfo?(ruturaj)
Updated the line #10.

Strange, the eslinter nor `./mach eslint` didn't show any issues.
Attachment #8876189 - Attachment is obsolete: true
Flags: needinfo?(ruturaj)
Attachment #8876571 - Flags: review+
should I assign the checkin-needed flag?
Flags: needinfo?(ntim.bugs)
(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).
Flags: needinfo?(ntim.bugs)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a33646645e
Add network monitor tests for searchbox autocomplete. r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54a33646645e
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.