1364113 - Test cases for Network Monitor autocomplete

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Comment 1

[Tim Nguyen :ntim]

Attachment: autocomplete-tests.patch

Ruturaj, would you be interested in taking this one ?

I've attached a test boilerplate if that helps.

You can run the test using:

"./mach mochitest devtools/client/shared/components/test/mochitest/test_searchbox.html"

Comment 2

[Ruturaj Vartak]

yes.. thanks :)

Comment 3

[Ruturaj Vartak]

Hey Tim, trying out KeyboardEvents... what is wrong here ?
```

    // Start keyboardEvent Tests
    $(".devtools-searchinput").focus();
    await forceRender(component); // Wait for state update
    // ArrowDown
    $(".devtools-searchinput").dispatchEvent(
      new KeyboardEvent("keydown", {key: "ArrowDown"})
    );
    await forceRender(component); // Wait for state update
    is(refs.autocomplete.state.selectedIndex, 0, "selectedIndex is 0");

```

The test fails, it seems keyboard event isn't being sent.

Comment 4

[Tim Nguyen :ntim]

EventUtils.synthesizeKey("VK_DOWN", {}); 

seems like what you need.

Comment 5

[Ruturaj Vartak]

Attachment: fix-1364113-1.patch

Hi Tim,
Implemented the following test cases.

[SearchBox]
- SearchBox setup
- initial empty value
- initially notFocussed
- setting value to inputbox
- checking for focus on the input
- onBlur
- ClearButton hidden initialized
- ClearButton shown on some value
KeyDown emptyAutoComplete return undefined
- ArrowDown
- ArrowUp
- PageDown
- PageUp
- Home
- End
- Tab
	- and hide box
- Backspace
	- reactivates popup
- Enter
	- and hide box
- Escape

[AutocompletePopup]
- initialy DOM invisibility
- focus on input and visibility
- List compare
- Initial selectedIndex = -1
- Blur hiding of autocomplete
- filtering of Values
- jumpToTop (by key events)
- jumpToBottom (by key events)
- jumpBy (by key events)
- select (by key events)
- Selection class
- MouseDown

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8867461 [details] [diff] [review]
fix-1364113-1.patch

Julian, can you look at this please ?

Thanks

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8867461 [details] [diff] [review]
fix-1364113-1.patch

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

I haven't gone into detail for this review, but you might want to split the autocomplete tests in smaller bits:
- the items in the list
- keyboard shortcut tests
- mouse test

::: devtools/client/shared/components/test/mochitest/test_searchbox.html
@@ +157,5 @@
> +    await forceRender(component); // Wait for state update
> +    sendString("aB");
> +    await forceRender(component); // Wait for state update
> +    is(refs.autocomplete.state.filteredList.length,
> +      2, "Filtering works (case insensitive)");

Rather than checking against the list length, you can use compareAutocompleteList directly.

Comment 8

[Ruturaj Vartak]

Attachment: fix-1364113-2.patch

- split the testcases in 2 files
-- pure SearchBox component tests
-- SearchBox with AutocompletePopup component tests

Comment 9

[Ruturaj Vartak]

Hey Tim,

After I do this...

    // Escape should remove the autocomplete component
    synthesizeKey("VK_ESCAPE", {});
    await forceRender(component); // Wait for state update
    ok(!$(".devtools-autocomplete-popup"),
      "Autocomplete list removed from DOM on Escape");

    // All the above works fine....


If I do, 

    // Click on input should activate the autocomplete
    synthesizeMouse(
      $(".devtools-searchinput"),
      5, 5,
      {}
    );
    await forceRender(component); // Wait for state update
    is(component.state.focused, true, "focussed");


OR if I do $(".devtools-searchinput").focus();

Neither of the above 2 methods lead to generation of Popup. Am I doing anything wrong?

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8867520 [details] [diff] [review]
fix-1364113-2.patch

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

Looks good! Can you submit a new version at least fixing the OSX issue with ctrlKey?
Then we can push to try and see if everything's ok.

Also let me know what were your plans with the empty test method. Based on that, clearing the review flag for now.

::: devtools/client/shared/components/test/mochitest/test_searchbox-with-autocomplete.html
@@ +3,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<!DOCTYPE html>
> +<html>
> +<!--
> +Test the searchbox component

nit: Test the searchbox and autocomplete-popup components

@@ +19,5 @@
> +<script>
> +/* import-globals-from head.js */
> +"use strict";
> +window.onload = async function () {
> +  async function createComponentTest(factory, props) {

JS Doc for this helper (consider moving it to head.js since you use it in two tests)

@@ +33,5 @@
> +      $: (s) => container.querySelector(s),
> +    };
> +  }
> +
> +  function compareAutocompleteList(list, reference) {

Can you add some JS doc for this helper ?

@@ +81,5 @@
> +    is(refs.autocomplete.state.selectedIndex, -1, "Initialised selectedIndex is -1");
> +
> +    // Blur event
> +    $(".devtools-searchinput").blur();
> +    await forceRender(component); // Wait for state update

I don't think we should duplicate this "// Wait for state update" everywhere

@@ +107,5 @@
> +    synthesizeKey("VK_DOWN", {});
> +    await forceRender(component); // Wait for state update
> +    is(refs.autocomplete.state.selectedIndex, 0, "selectedIndex is 0");
> +    ok($(".devtools-autocomplete-listbox")
> +      .childNodes[0].className.includes("autocomplete-selected"),

can we void the childNodes[0]? $(".autocomplete-item:nth-child(1)")?

@@ +130,5 @@
> +    synthesizeKey("VK_HOME", {});
> +    await forceRender(component); // Wait for state update
> +    is(refs.autocomplete.state.selectedIndex, 0, "Home works");
> +
> +    // End should take at the bottom

nit: at the bottom -> to the bottom of the list

@@ +134,5 @@
> +    // End should take at the bottom
> +    synthesizeKey("VK_END", {});
> +    await forceRender(component); // Wait for state update
> +    is(refs.autocomplete.state.selectedIndex, 6, "End works");
> +    synthesizeKey("VK_DOWN", {});

Can you separate this from the code above with a blank line? 
Feels like this is a part of "End should take at the bottom" while it's really not.

@@ +160,5 @@
> +    synthesizeKey("VK_BACK_SPACE", {});
> +    synthesizeKey("VK_BACK_SPACE", {});
> +    sendString("pq");
> +    synthesizeMouse(
> +      $(".devtools-autocomplete-listbox").childNodes[0],

same comment, avoid childNodes[0]

@@ +166,5 @@
> +      {}
> +    );
> +    await forceRender(component); // Wait for state update
> +    is(component.state.value, "pqr", "Mouse click selects the item");
> +    ok(!$(".devtools-autocomplete-popup"), "Tab hit hides the popup");

Comment is wrong, we are not selecting with tab

@@ +172,5 @@
> +    // Escape should remove the autocomplete component
> +    synthesizeKey("VK_ESCAPE", {});
> +    await forceRender(component); // Wait for state update
> +    ok(!$(".devtools-autocomplete-popup"),
> +      "Autocomplete list removed from DOM on Escape");

I don't know if you are talking about this part in your last comment in the bug. 
But if you hit ESCAPE here, the field still contains "pqr", which is already an autocomplete value. Hence no popup when you focus again.

Something useful when you need to play with this kind of tests is to pause the test (for instance using `await new Promise(r => setTimeout(r, 60000));`). Then you can interact with the component yourself and see what's happening.

@@ +175,5 @@
> +    ok(!$(".devtools-autocomplete-popup"),
> +      "Autocomplete list removed from DOM on Escape");
> +  }
> +
> +  async function mouseEventsWithAutocomplete() {

Remove this empty test, or implement if you had something in mind?

::: devtools/client/shared/components/test/mochitest/test_searchbox.html
@@ +19,5 @@
> +<script>
> +/* import-globals-from head.js */
> +"use strict";
> +window.onload = function () {
> +  async function createComponentTest(factory, props) {

Same as my comment in the other test: JSDoc + consider moving it to head.js

@@ +54,5 @@
> +    ok($(".devtools-searchinput-clear").hidden, "ClearButton hidden");
> +    ok($(".devtools-searchinput").placeholder, "crazy placeholder",
> +      "ClearButton hidden");
> +
> +    synthesizeKey("f", {ctrlKey: 1});

instead of ctrlKey: 1, you should use accelKey: true. accelKey will map to CTRL on Windows/Linux and to CMD on OSX.

Comment 11

[Ruturaj Vartak]

Attachment: fix-1364113-3.patch

Hey Julian,

Thanks for the detailed review, the await setTimeout really helped.

Here are the fixes...
- fix nit message for test
- moved createComponentTest to head.js and added jsdoc comments
- added jsdoc comments for compareAutocompleteList
- kept comment // Wait for state update only for the 1st command in file
- used nth-child selector instead of childNodes
- fixed nit: at the bottom 
- fixed nit: separation: added extra \n for VK_END
- fixed nit: Tab comment fix
- Empty Mouse test filled-in

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8868007 [details] [diff] [review]
fix-1364113-3.patch

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

Thanks for addressing my comments! 

This should be almost ready to land (missed a few things in my first review, sorry about that).
Have a look at my comments, and also can you add my handle to the reviewers in your changeset summary?

I pushed your changeset to try with an additional changeset to use synthesizeMouseAtCenter instead of synthesizeMouse.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=283b5f51c49746e811f291423f5272866239de06

::: devtools/client/shared/components/test/mochitest/test_searchbox-with-autocomplete.html
@@ +53,5 @@
> +      "ABC",
> +    ],
> +    onChange: () => null,
> +  });
> +  const refs = component.refs;

nit: can be shortened to const {refs} = component.

@@ +166,5 @@
> +    // ArrowDown
> +    synthesizeKey("VK_DOWN", {});
> +    await forceRender(component);
> +    // Click on input should activate the autocomplete
> +    synthesizeMouse(

Did you know about synthesizeMouseAtCenter? On my machine the following seems to work:

synthesizeMouseAtCenter($(".devtools-searchinput"), {}, window);

Unless there was a specific reason for specifying the 5, 5 click coordinates, let's use this method instead.

@@ +175,5 @@
> +    await forceRender(component);
> +    is(component.state.focused, true, "focussed");
> +    sendString("pq");
> +    await forceRender(component);
> +    synthesizeMouse(

same comment about synthesizeMouseAtCenter vs synthesizeMouse

@@ +188,5 @@
> +
> +  add_task(async function () {
> +    await testSearchBoxWithAutocomplete();
> +    await testKeyEventsWithAutocomplete();
> +    await mouseEventsWithAutocomplete();

for consistency let's rename this to testMouseEventsWithAutocomplete

::: devtools/client/shared/components/test/mochitest/test_searchbox.html
@@ +39,5 @@
> +    ok($(".devtools-searchinput-clear").hidden, "ClearButton hidden");
> +    ok($(".devtools-searchinput").placeholder, "crazy placeholder",
> +      "ClearButton hidden");
> +
> +    synthesizeKey("f", {accelKey: 1});

I know `1 == true`, but I would really prefer if we could stick with an actual boolean, for consistency with the rest of the codebase.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8868007 [details] [diff] [review]
fix-1364113-3.patch

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

::: devtools/client/shared/components/test/mochitest/test_searchbox-with-autocomplete.html
@@ +172,5 @@
> +      5, 5,
> +      {}
> +    );
> +    await forceRender(component);
> +    is(component.state.focused, true, "focussed");

nit: "Component should now be focused"

::: devtools/client/shared/components/test/mochitest/test_searchbox.html
@@ +35,5 @@
> +    });
> +
> +    is(component.state.value, "", "Initial value is blank");
> +    ok(!component.state.focused, "Input isn't initially focused");
> +    ok($(".devtools-searchinput-clear").hidden, "ClearButton hidden");

ClearButton is not a component, so can we use normal capitalization ("Clear button") ?

@@ +37,5 @@
> +    is(component.state.value, "", "Initial value is blank");
> +    ok(!component.state.focused, "Input isn't initially focused");
> +    ok($(".devtools-searchinput-clear").hidden, "ClearButton hidden");
> +    ok($(".devtools-searchinput").placeholder, "crazy placeholder",
> +      "ClearButton hidden");

Seems like "ClearButton hidden"); should be removed here.

@@ +41,5 @@
> +      "ClearButton hidden");
> +
> +    synthesizeKey("f", {accelKey: 1});
> +    await forceRender(component); // Wait for state update
> +    ok(component.state.focused, "Shortcut key focussed the input box");

nit: "focused"

@@ +45,5 @@
> +    ok(component.state.focused, "Shortcut key focussed the input box");
> +
> +    $(".devtools-searchinput").blur();
> +    await forceRender(component);
> +    ok(!component.state.focused, "onBlur focussed correctly set");

nit: "focused state set to false after blur"

@@ +57,5 @@
> +    is($(".devtools-searchinput").value, "foo", "value was properly set on element");
> +
> +    // text in input box should trigger showing ClearButton
> +    ok(!$(".devtools-searchinput-clear").hidden, "ClearButton shown");
> +    // Clearing value should remove ClearButton

ClearButton is not a component, so can we use normal capitalization ("Clear button") ?

@@ +62,5 @@
> +    await setState(component, {
> +      value: "",
> +    });
> +    await forceRender(component);
> +    ok($(".devtools-searchinput-clear").hidden, "ClearButton was removed");

Same here.

Comment 14

[Ruturaj Vartak]

Attachment: fix-1364113-4.patch

Thanks Julian and Tim,

- worked on nits suggested by you guys
- used synthesizeMouseAtCenter

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8868246 [details] [diff] [review]
fix-1364113-4.patch

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

Looks good to me, thanks a lot for adding the tests! 

One minor nit. 
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30ffdf28403f7d5fa3cc21eed187966014c3c081

Feel free to r+ directly the next version if you only apply my comment.

::: devtools/client/shared/components/test/mochitest/test_searchbox-with-autocomplete.html
@@ +183,5 @@
> +
> +  add_task(async function () {
> +    await testSearchBoxWithAutocomplete();
> +    await testKeyEventsWithAutocomplete();
> +    await mouseEventsWithAutocomplete();

can we rename this to testMouseEventsWithAutocomplete?

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8868246 [details] [diff] [review]
fix-1364113-4.patch

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

::: devtools/client/shared/components/test/mochitest/test_searchbox.html
@@ +36,5 @@
> +
> +    is(component.state.value, "", "Initial value is blank");
> +    ok(!component.state.focused, "Input isn't initially focused");
> +    ok($(".devtools-searchinput-clear").hidden, "Clear button hidden");
> +    ok($(".devtools-searchinput").placeholder, "crazy placeholder");

You probably meant to use "is" here:

is($(".devtools-searchinput").placeholder, "crazy placeholder",
   "Placeholder is properly set");

@@ +38,5 @@
> +    ok(!component.state.focused, "Input isn't initially focused");
> +    ok($(".devtools-searchinput-clear").hidden, "Clear button hidden");
> +    ok($(".devtools-searchinput").placeholder, "crazy placeholder");
> +
> +    synthesizeKey("f", {accelKey: true});

nit: { accelKey: true } (spacing)

Comment 17

[Ruturaj Vartak]

Attachment: fix-1364113-5.patch

Julian,
sorry I missed that earlier rename comment

Tim,
I've added that space and converted `ok` to `is`.
Also, eslint doesn't show error for that object-literal no-space, hence I missed it even in some earlier patches. We should get that added to eslint config.

Thanks guys, you have really sharp eyes :)

Comment 18

[Ruturaj Vartak]

Tim, Julian,

These test cases only do unit testing of the 2 React components. But shouldn't we have that for implemented version in Network monitor? 'Cause citing bug 1364093, bug 1364092... All the logic would be outside these 2 components.

I'm planning to work on all the bugs flagged as "depends on" in bug#1354504 - So.. 

- Should extend the SearchBox with NetMonitorSearchBox ? - with unit testing of NetMonitorSearchBox
OR
- Whatever we endup coding in netmonitor/src/components/toolbar.js for specifics of Network monitor search, we'll have to do a functional tests something like browser_net_filter-flags.js

Was waiting for this patch to slide-in master before I get on to those bugs.. thought might as well try to look ahead.

Inputs appreciated,
Thanks.

Comment 19

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #18)
> Tim, Julian,
> 
> These test cases only do unit testing of the 2 React components. But
> shouldn't we have that for implemented version in Network monitor? 'Cause
> citing bug 1364093, bug 1364092... All the logic would be outside these 2
> components.
>
> I'm planning to work on all the bugs flagged as "depends on" in bug#1354504
> - So.. 
> 
> - Should extend the SearchBox with NetMonitorSearchBox ? - with unit testing
> of NetMonitorSearchBox
> OR
> - Whatever we endup coding in netmonitor/src/components/toolbar.js for
> specifics of Network monitor search, we'll have to do a functional tests
> something like browser_net_filter-flags.js

You can try to allow specifying a function for the autocomplete list in the searchbox and autocomplete popup props.
Something like this:

autocompleter: (value) => {
  let filters = [
    ...HEADER_FILTERS.map(f => f + ":"),
    ...HEADER_FILTERS.map(f => "-" + f + ":")
  ];

  if (value starts with " ") {
    return filters.map(f => value + f);
  }
  
  ...

  return filters;
  
}

Or if you think it may require more logic than that, you could make it accept an AutocompleteProvider class.

Comment 20

[Tim Nguyen :ntim]

> Also, eslint doesn't show error for that object-literal no-space, hence I missed it even in some earlier patches. We should get that added to eslint config.

There's a bug filed about that, but I haven't got time to finish it, feel free to do it though: bug 1311078.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baef5b5701f9
Test cases for Network Monitor autocomplete. r=jdescottes, ntim

Comment 22

[Tim Nguyen :ntim]

This accidentally landed in bug 1354504, but I've backed it out and re-landed here.

Please be careful with bug numbers next time :)

Comment 23

[Ruturaj Vartak]

Hey Tim,
Sorry I triggered incorrect bug!

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/baef5b5701f9