1404851 - Migrate browser_webconsole_bug_651501_document_body_autocomplete.js to the new frontend

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1404851 - Rename, adapt and enable browser_webconsole_document_body_autocomplete; .

We drop the part of the test which tests the output of document.body
since it's already tested in reps.

Review commit: https://reviewboard.mozilla.org/r/203854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203854/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8932801 [details]
Bug 1404851 - Rename, adapt and enable browser_webconsole_document_body_autocomplete; .

https://reviewboard.mozilla.org/r/203854/#review209346

Looks good to me, thanks!

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:6
(Diff revision 1)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  // Tests that document.body autocompletes in the web console. See Bug 651501.

It looks like the reason for this test is that document.body is actually a native getter and we used not to provide autocompletion on native getters (risk of side effects etc...). Maybe mention that in the test's main comment, otherwise it's hard to understand why we have this test on top of browser_jsterm_completion.js and browser_jsterm_autocomplete_escape_key.js

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:18
(Diff revision 1)
> +  let { jsterm } = await openNewTabAndConsole(TEST_URI);
>  
> -add_task(function* () {
> -  yield loadTab(TEST_URI);
> +  const {
> +    autocompletePopup: popup,
> +    completeNode,
> +    inputNode,

eslint: inputNode is not used

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:33
(Diff revision 1)
> -
> -    is(popup.itemCount, jsterm._autocompleteCache.length,
> +  is(popup.itemCount, jsterm._autocompleteCache.length, "popup.itemCount is correct");
> +  ok(jsterm._autocompleteCache.includes("addEventListener"),
> -       "popup.itemCount is correct");
> -    isnot(jsterm._autocompleteCache.indexOf("addEventListener"), -1,
> -          "addEventListener is in the list of suggestions");
> +        "addEventListener is in the list of suggestions");
> -    isnot(jsterm._autocompleteCache.indexOf("bgColor"), -1,
> +  ok(jsterm._autocompleteCache.includes("bgColor"), "bgColor is in the list of suggestions");

eslint: > 90chars

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:34
(Diff revision 1)
> -    is(popup.itemCount, jsterm._autocompleteCache.length,
> +  ok(jsterm._autocompleteCache.includes("addEventListener"),
> -       "popup.itemCount is correct");
> -    isnot(jsterm._autocompleteCache.indexOf("addEventListener"), -1,
> -          "addEventListener is in the list of suggestions");
> +        "addEventListener is in the list of suggestions");
> -    isnot(jsterm._autocompleteCache.indexOf("bgColor"), -1,
> -          "bgColor is in the list of suggestions");
> +  ok(jsterm._autocompleteCache.includes("bgColor"), "bgColor is in the list of suggestions");
> +  ok(jsterm._autocompleteCache.includes("ATTRIBUTE_NODE"), "ATTRIBUTE_NODE is in the list of suggestions");

eslint: > 90chars

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:46
(Diff revision 1)
> -  });
> -
>    let inputStr = "document.b";
>    jsterm.setInputValue(inputStr);
>    EventUtils.synthesizeKey("o", {});
>    let testStr = inputStr.replace(/./g, " ") + " ";

nit: move this variable right before the `is(completeNode.value, testStr + [...])` as it is only needed on this line. 
I would also be fine with having just "           dy" (+ a comment) for this test rather than having a regexp.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932801 [details]
Bug 1404851 - Rename, adapt and enable browser_webconsole_document_body_autocomplete; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203854/diff/1-2/

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the review Julian. I renamed the test so it is more obvious what it does, and added some comments.
Pushed to TRY before landing

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a91f7382c96
Rename, adapt and enable browser_webconsole_document_body_autocomplete; r=jdescottes.

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4a91f7382c96