Closed Bug 1326410 Opened 3 years ago Closed 3 years ago

Fix ESlint errors in devtools/shared/transport/tests/unit

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: ntim, Assigned: ruturaj)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(2 files, 4 obsolete files)

Run `./mach eslint devtools/shared/transport/test/unit/ --no-ignore --fix` to see the errors.
Priority: -- → P3
Hey Tim,

I ran the command - I'm at master@54c460dbd


=========================================
~/code/gecko-dev$ ./mach eslint devtools/shared/transport/test/unit/ --no-ignore --fix
✖ 0 problems (0 errors, 0 warnings)
Flags: needinfo?(ntim.bugs)
I forgot to mention, there are many ESLint errors poping when individually opening the file. ie. head_dbg.js has many errors - Many variables assigned but never used type.
There was a typo in my first comment: 

./mach eslint devtools/shared/transport/tests/unit/ --no-ignore --fix

(Should be tests with an "s")

(In reply to Ruturaj Vartak from comment #2)
> I forgot to mention, there are many ESLint errors poping when individually
> opening the file. ie. head_dbg.js has many errors - Many variables assigned
> but never used type.

Just check if they are used inside the test directory (just search VariableName path:devtools/shared/transport/tests/unit in dxr). If they are used, then use /* exported varName1, varName2 */ to mark them as used.
Flags: needinfo?(ntim.bugs)
Assignee: nobody → ruturaj
(In reply to Ruturaj Vartak from comment #4)
> So.. I'm just going ahead using a search like this one
> https://dxr.mozilla.org/mozilla-central/
> search?q=path%3Adevtools%2Fshared%2Ftransport%2Ftests%2Funit+prop-ref%3Adefer

Nice find! didn't know about prop-ref
Attached patch fix-1326410-1.patch (obsolete) — Splinter Review
- Fixed suggestions by ESLint

Errors still present
- "use strict" - Shows as unused useless variables.
- test_client_server_bulk.js - Not sure how I can fix more than 3 nested fn calls (Line: 165-166)
- test_dbgsocket.js - `transport` even though shows as unused is used, should I add to exported ?

I tested using ./mach test command

Command output
========================================
~/code/gecko-dev$ ./mach test devtools/shared/transport/tests/unit/
 0:00.10 LOG: MainThread INFO Using at most 16 threads.
 0:00.10 SUITE_START: MainThread 10
 0:00.11 TEST_START: Thread-2 devtools/shared/transport/tests/unit/test_client_server_bulk.js
 0:00.14 TEST_START: Thread-1 devtools/shared/transport/tests/unit/test_bulk_error.js
 0:00.15 TEST_START: Thread-8 devtools/shared/transport/tests/unit/test_queue.js
 0:00.15 TEST_START: Thread-3 devtools/shared/transport/tests/unit/test_dbgsocket.js
 0:00.15 TEST_START: Thread-7 devtools/shared/transport/tests/unit/test_packet.js
 0:00.19 TEST_START: Thread-5 devtools/shared/transport/tests/unit/test_delimited_read.js
 0:00.20 TEST_START: Thread-10 devtools/shared/transport/tests/unit/test_transport_events.js
 0:00.21 TEST_START: Thread-6 devtools/shared/transport/tests/unit/test_no_bulk.js
 0:00.22 TEST_START: Thread-4 devtools/shared/transport/tests/unit/test_dbgsocket_connection_drop.js
 0:00.23 TEST_START: Thread-9 devtools/shared/transport/tests/unit/test_transport_bulk.js
 0:01.16 TEST_END: Thread-5 PASS
 0:01.30 TEST_END: Thread-7 PASS
 0:01.42 TEST_END: Thread-6 PASS
 0:01.48 TEST_END: Thread-1 PASS
 0:01.55 TEST_END: Thread-10 PASS
 0:01.58 TEST_END: Thread-9 PASS
 0:01.65 TEST_END: Thread-4 PASS
 0:01.65 TEST_END: Thread-8 PASS
 0:01.74 TEST_END: Thread-3 PASS
 0:01.87 TEST_END: Thread-2 PASS
 0:01.87 LOG: MainThread INFO INFO | Result summary:
 0:01.87 LOG: MainThread INFO INFO | Passed: 10
 0:01.87 LOG: MainThread INFO INFO | Failed: 0
 0:01.87 LOG: MainThread INFO INFO | Todo: 0
 0:01.87 LOG: MainThread INFO INFO | Retried: 0
 0:01.87 SUITE_END: MainThread 
Summary
=======

Ran 10 tests
Expected results: 10
Unexpected results: 0

OK
Attachment #8830236 - Flags: review?(ntim.bugs)
Comment on attachment 8830236 [details] [diff] [review]
fix-1326410-1.patch

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

Thanks for the patch!

I don't see how adding "use strict"; at the beginning of each file causes unused variables. If you do exactly that there should be no issue.

As for test_client_server_bulk.js, here's how I've rewritten the code, let me know if the tests work:


var test_bulk_request_cs = Task.async(function* (transportFactory, actorType, replyType) {
  // Ensure test files are not present from a failed run
  cleanup_files();
  writeTestTempFile("bulk-input", really_long());

  let transport = yield transportFactory();

  let client = new DebuggerClient(transport);
  let [, traits] = yield client.connect();
  do_check_eq(traits.bulk, true);

  let response = yield client.listTabs();
  
  let serverDeferred = defer();
  let bulkCopyDeferred = defer();

  let request = client.startBulkRequest({
    actor: response.testBulk,
    type: actorType,
    length: really_long().length
  });


  // Send bulk data to server
  request.on("bulk-send-ready", ({copyFrom}) => {
    NetUtil.asyncFetch({
      uri: NetUtil.newURI(getTestTempFile("bulk-input")),
      loadUsingSystemPrincipal: true
    }, async input => {
      await copyFrom(input);
      input.close();
      bulkCopyDeferred.resolve();
    });
  });
  
  // Set up reply handling for this type
  yield replyHandlers[replyType](request);
  client.close();
  transport.close();

  DebuggerServer.on("connectionchange", (event, type) => {
    if (type === "closed") {
      serverDeferred.resolve();
    }
  });

  return promise.all([
    bulkCopyDeferred.promise,
    serverDeferred.promise
  ]);
});

::: devtools/shared/transport/tests/unit/head_dbg.js
@@ +3,5 @@
>  
>  "use strict";
>  
> +/* exported Cr CC NetUtil defer errorCount initTestDebuggerServer
> +            writeTestTempFile socket_transport local_transport really_long

We usually separate globals with commas:

/* exported Cr, CC, NetUtils, defer, ... */

@@ +21,4 @@
>  const { Task } = require("devtools/shared/task");
>  
>  const Services = require("Services");
> +// const DevToolsUtils = require("devtools/shared/DevToolsUtils");

Since this is unused, it can be removed altogether.

@@ +64,4 @@
>      try {
>        // If we've been given an nsIScriptError, then we can print out
>        // something nicely formatted, for tools like Emacs to pick up.
> +      // var scriptError = message.QueryInterface(Ci.nsIScriptError);

We still want to call message.QueryInterface()

So replacing this with: message.QueryInterface(Ci.nsIScriptError); is enough (without assigning it to a variable).

::: devtools/shared/transport/tests/unit/test_dbgsocket.js
@@ +53,5 @@
>    let closedDeferred = defer();
>    transport.hooks = {
> +    onPacket: function (packet) {
> +      this.onPacket = function (packet2) {
> +        do_check_eq(packet2.unicode, unicodeString);

I'd prefer doing:
this.onPacket = function ({unicode}) {
  do_check_eq(unicode, unicodeString);
};

::: devtools/shared/transport/tests/unit/test_no_bulk.js
@@ +20,4 @@
>  /** * Tests ***/
>  
>  var test_bulk_send_error = Task.async(function* (transportFactory) {
> +  // let deferred = defer();

Can you remove this ?
Attachment #8830236 - Flags: review?(ntim.bugs)
(In reply to Ruturaj Vartak from comment #6)
> - test_dbgsocket.js - `transport` even though shows as unused is used,
> should I add to exported ?

I would just not assign the code to a variable:

try {
  yield DebuggerClient.socketConnect({
    host: "127.0.0.1",
    port: gPort
  });
}
The rewrite of "test_client_server_bulk.js" seems is breaking the test, the test cases don't end, I pressed Ctrl+C to end it.

Let me have a go at it and find some soln.
Attached image use-strict-eslint.png
the "use strict" error that I mentioned. Is it that my Sublime text is incorrectly configured?
Attached patch fix-1326410-2.patch (obsolete) — Splinter Review
- Worked on your suggestions
- Tried another approach at multi-nested fn calls issue.

- ./mach test devtools/shared/transport/tests/unit/ - working fine.
Attachment #8830236 - Attachment is obsolete: true
Attachment #8831090 - Flags: review?(ntim.bugs)
(In reply to Ruturaj Vartak from comment #10)
> Created attachment 8831075 [details]
> use-strict-eslint.png
> 
> the "use strict" error that I mentioned. Is it that my Sublime text is
> incorrectly configured?

Probably the case. Do these errors show up with `./mach eslint devtools/shared/transport/tests/unit --no-ignore` ? if not, you should be fine. If they do appear, try running `./mach eslint --setup`
Comment on attachment 8831090 [details] [diff] [review]
fix-1326410-2.patch

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

f+ with "use strict" added.
Attachment #8831090 - Flags: review?(ntim.bugs) → feedback+
Im out on a vacation, will check as soon as i get back. Thanks
Attached patch fix-1326410-3.patch (obsolete) — Splinter Review
Hi,
The command `./mach eslint devtools/shared/transport/tests/unit --no-ignore` isn't returning "no strict" errors. So I'll ignore.

I had forgotten the `transport` variable fix, Adding that.
however there is still a silly issue that exists (All tests are passing)
===============================================
$ ./mach eslint devtools/shared/transport/tests/unit --no-ignore
/home/rutu/code/gecko-dev/devtools/shared/transport/tests/unit/test_dbgsocket.js
  99:12  error  Unnecessary 'else' after 'return'.  no-else-return (eslint)

✖ 1 problem (1 error, 0 warnings)

============================================

It seems `return` is necessary, I tried commenting it - the tests fail.
Attachment #8831090 - Attachment is obsolete: true
Attachment #8833850 - Flags: review?(ntim.bugs)
(In reply to Ruturaj Vartak from comment #15)
> Created attachment 8833850 [details] [diff] [review]
> fix-1326410-3.patch
> 
> Hi,
> The command `./mach eslint devtools/shared/transport/tests/unit --no-ignore`
> isn't returning "no strict" errors. So I'll ignore.
I'm surprised about this. I do see a bunch of "Use the global form of 'use strict'." locally. The fix is quite simple, you can just add the line "use strict"; at the beginning of the affected files. I can fix it before landing if you prefer.

> I had forgotten the `transport` variable fix, Adding that.
> however there is still a silly issue that exists (All tests are passing)
> ===============================================
> $ ./mach eslint devtools/shared/transport/tests/unit --no-ignore
> /home/rutu/code/gecko-dev/devtools/shared/transport/tests/unit/
> test_dbgsocket.js
>   99:12  error  Unnecessary 'else' after 'return'.  no-else-return (eslint)
> 
> ✖ 1 problem (1 error, 0 warnings)
> 
> ============================================
> 
> It seems `return` is necessary, I tried commenting it - the tests fail.

The `return` is necessary but not the `else`.

    if (e.result == Cr.NS_ERROR_CONNECTION_REFUSED ||
        e.result == Cr.NS_ERROR_NET_TIMEOUT) {
      // The connection should be refused here, but on slow or overloaded
      // machines it may just time out.
      do_check_true(true);
      return;
    } else {
      throw e;
    }

is equivalent to:

    if (e.result == Cr.NS_ERROR_CONNECTION_REFUSED ||
        e.result == Cr.NS_ERROR_NET_TIMEOUT) {
      // The connection should be refused here, but on slow or overloaded
      // machines it may just time out.
      do_check_true(true);
      return;
    }
    throw e;
(In reply to Tim Nguyen :ntim from comment #16)
> (In reply to Ruturaj Vartak from comment #15)
> > Created attachment 8833850 [details] [diff] [review]
> > fix-1326410-3.patch
> > 
> > Hi,
> > The command `./mach eslint devtools/shared/transport/tests/unit --no-ignore`
> > isn't returning "no strict" errors. So I'll ignore.
> I'm surprised about this. I do see a bunch of "Use the global form of 'use
> strict'." locally. The fix is quite simple, you can just add the line "use
> strict"; at the beginning of the affected files. I can fix it before landing
> if you prefer.

Actually, your patch already fixes all the use strict warnings. So you don't need to do anything here.
Attached patch fix-1326410-4.patch (obsolete) — Splinter Review
Fixed the else-catch error as well as per ur suggestion.
Attachment #8833850 - Attachment is obsolete: true
Attachment #8833850 - Flags: review?(ntim.bugs)
Attachment #8833954 - Flags: review?(ntim.bugs)
Comment on attachment 8833954 [details] [diff] [review]
fix-1326410-4.patch

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

Looks great, thanks!

Can you remove devtools/shared/transport/tests/unit from .eslintignore ?

https://dxr.mozilla.org/mozilla-central/source/.eslintignore#117

::: devtools/shared/transport/tests/unit/test_dbgsocket_connection_drop.js
@@ +62,5 @@
>    });
>    let closedDeferred = defer();
>    transport.hooks = {
> +    onPacket: function (packet) {
> +      this.onPacket = function (packet2) {

packet2 can be removed, since it doesn't seem to be used.
Attachment #8833954 - Flags: review?(ntim.bugs) → review+
Please find the suggested fix
Attachment #8833954 - Attachment is obsolete: true
Attachment #8833986 - Flags: review?(ntim.bugs)
Comment on attachment 8833986 [details] [diff] [review]
fix-1326410-5.patch

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

Thanks!
Attachment #8833986 - Flags: review?(ntim.bugs) → review+
Thanks Tim !
https://hg.mozilla.org/mozilla-central/rev/45b79b251816
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.