1447944 - Enable ESLint rule no-undef for test files in devtools/server/tests/unit/

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Mark Banner (:standard8)]

no-undef is a very useful rule to have, as part of bug 1230193 we should continue the roll-out - for this directory it has also found some issues in the tests themselves.

I have a patch for this.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1447944 - Enable ESLint rule no-undef for test files in devtools/server/tests/unit/.

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

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1447944 - Remove devtool's check_except helper function in favour of Assert.throws. .

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

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8961319 [details]
Bug 1447944 - Enable ESLint rule no-undef for test files in devtools/server/tests/unit/.

https://reviewboard.mozilla.org/r/230124/#review236048

Thanks for the cleanup and thanks for also fixing the tests that were just plain wrong.

::: devtools/server/tests/unit/test_protocol_simple.js:185
(Diff revision 1)
> -    let badActor = ActorClassWithSpec({}, {
> -      missing: preEvent("missing-event", function() {
> +    let badActor = protocol.ActorClassWithSpec({}, {
> +      missing: protocol.preEvent("missing-event", function() {
>        })

If I understand correctly, this test was not even correctly asserting anything! Thanks for fixing this.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8961319 [details]
Bug 1447944 - Enable ESLint rule no-undef for test files in devtools/server/tests/unit/.

https://reviewboard.mozilla.org/r/230124/#review236048

> If I understand correctly, this test was not even correctly asserting anything! Thanks for fixing this.

Yes, exactly right, the items were undefined (I think it had been this way since it landed).

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8961320 [details]
Bug 1447944 - Remove devtool's check_except helper function in favour of Assert.throws. .

https://reviewboard.mozilla.org/r/230126/#review236052

Looks good to me, it's much better to assert the actual error message!

::: devtools/server/tests/unit/test_dbgglobal.js:13
(Diff revision 1)
>    // before we initialize it...
> -  check_except(function() {
> -    DebuggerServer.createListener();
> -  });
> -  check_except(DebuggerServer.closeAllListeners);
> -  check_except(DebuggerServer.connectPipe);
> +  Assert.throws(() => DebuggerServer.createListener(),
> +    /DebuggerServer has not been initialized/,
> +    "createListener should throw before it has been initialized");
> +  Assert.throws(DebuggerServer.closeAllListeners,
> +    /this is undefined/,

I think all those exceptions should be meaningful ones and not "this is undefined", which makes it clear thanks to using Assert.throws. I will log a follow up!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c96061263e3a
Enable ESLint rule no-undef for test files in devtools/server/tests/unit/. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a064dac7fd39
Remove devtool's check_except helper function in favour of Assert.throws. r=jdescottes.

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c96061263e3a
https://hg.mozilla.org/mozilla-central/rev/a064dac7fd39