Closed
Bug 1368431
Opened 7 years ago
Closed 7 years ago
Add network monitor tests for searchbox autocomplete
Categories
(DevTools :: Netmonitor, enhancement)
DevTools
Netmonitor
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ntim, Assigned: ruturaj)
References
Details
Attachments
(1 file, 3 obsolete files)
4.11 KB,
patch
|
ruturaj
:
review+
|
Details | Diff | Splinter Review |
We have a test for the component, but no test for the network monitor feature.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ruturaj
Assignee | ||
Comment 1•7 years ago
|
||
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)
Reporter | ||
Updated•7 years ago
|
Attachment #8872967 -
Attachment mime type: application/javascript → text/javascript
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8872967 -
Attachment is obsolete: true
Attachment #8876052 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
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)
Reporter | ||
Comment 6•7 years ago
|
||
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+
Reporter | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•7 years ago
|
||
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
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
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+
Assignee | ||
Comment 11•7 years ago
|
||
should I assign the checkin-needed flag?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 12•7 years ago
|
||
(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
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/54a33646645e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•