fix: Improve visibility of message to enable "Remote Debugging via USB" with web-ext run -t firefox-android #2038

sonalprabhu · 2020-10-02T10:24:15Z

Fixes #1969
fix: Looped the "Check Remote Debugging is ON" warning and highlighted it

src/extension-runners/firefox-android.js

src/util/adb.js

coveralls · 2020-10-02T11:19:34Z

Coverage remained the same at 100.0% when pulling 3b5e1ca on sonalprabhu:loop-warning into 82c4250 on mozilla:master.

rpl · 2020-10-02T11:32:36Z

@sonalprabhu as a side note, don't worry about the audit-deps failure in the travis CI job, it is unrelated to the changes in this PR, we'll take care of that and you'll just need to rebase this PR once we merged a fix for it.

Co-authored-by: Renovate Bot <bot@renovateapp.com>

…la#2034) Co-authored-by: Renovate Bot <bot@renovateapp.com>

Co-authored-by: Renovate Bot <bot@renovateapp.com>

…zilla#2039)

rpl

@sonalprabhu good job, this looks almost ready, there are just a few more tweaks needed (and a small fix to the test case, which is currently not failing even if it is expecting the wrong log level).

tests/unit/test-util/test.adb.js

src/util/adb.js

rpl · 2020-10-08T20:50:29Z

src/util/adb.js


    while (rdpUnixSockets.length === 0) {
+      log.info(`\n${msg}\n`);


I think that we can remove the additional '\n' from this log message, they were meant to make that log to stand out and be more visible (which was clearly not enough :-P), now we are going to keep printing this message until we find the unix socket file and so we don't need this additional space.

Let's just change this into log.info(msg).

rpl · 2020-10-08T20:52:36Z

tests/unit/test-util/test.adb.js

@@ -1129,6 +1131,44 @@ describe('utils/adb', () => {
      );
    });

+    it('informs the user if remote debugging is not enabled', async () => {


Nit, we don't really check if the remote debugging is enabled, we just wait for it to be available and so the description of this test may be a bit misleading, how about: it('reminds the user to enable remote debugging', ... instead?

rpl · 2020-10-08T21:01:38Z

tests/unit/test-util/test.adb.js

+      consoleStream.stopCapturing();
+      consoleStream.flushCapturedLogs();
+      consoleStream.startCapturing();
+      consoleStream.makeVerbose();


I think that the following tweaked version should be enough and slightly better:

consoleStream.flushCapturedLogs(); <--- clear the captured logs consoleStream.makeVerbose(); <--- make the log verbose (to be able to assert the log level) consoleStream.startCapturing(); <--- make sure that we start capturing logs ... const promise = ... <-- get the promise await assert.isRejected(promise, WebExtError); <--- wait the async function to complete ... // get the message that we expect by filtering the captured logs consoleStream.stopCapturing(); <--- stop capturing we don't need it anymore ... // assertions on the expected message and log level

rpl · 2020-10-08T21:03:37Z

tests/unit/test-util/test.adb.js

+      const maxDiscoveryTime = 50;
+      const retryInterval = 10;
+


Nit, let's remove this two consts and just pass the value in the call to adb.discoverRDPUnixSocket (we are not using them for any assertion and so we can make this test a bit shorter by making this small optional change).

rpl · 2020-10-08T21:11:32Z

tests/unit/test-util/test.adb.js

+
+      assert.ok(capturedMessages.some(
+        (message) => (
+          message.match('[debug]') &&


Surprisingly this assertion doesn't seem to be fully working, the log we are expecting is emitted with log.info and so this test should actually fail and pass if we use '[info]' instead.

Another thing that we may also take care of is to make it clear when this test fails if the message wasn't found or wasn't emitted with the right log level, at the moment the two conditions are checked by a single assertion and so a failure would make it immediately clear.

Let's change this to get the message that we want using something like const foundMessage = consoleStream.caputedMessages.find((message) => message.includes(expectedMessage)), then check the two conditions in separate assertions:

assert.ok(foundMessage); assert.ok(foundMessage && foundMessage.includes(expectedLogLevel));

(NOTE: the foundMessage && foundMessage.includes is mainly needed to make the flow type checker happy ;-))

rpl · 2020-10-09T10:09:21Z

@sonalprabhu the last CI jobs did fail because of some trailing spaces in test.adb.js (added when you added back the empty line that was previously there):

https://travis-ci.org/github/mozilla/web-ext/jobs/734232572#L246

You can fix that issue manually (by removing the trailing spaces and leave it as just an empty line as it was originally) or by running eslint --fix on that test file.

…into loop-warning

rpl · 2020-10-09T11:48:20Z

Can I create a new pull request? I am facing some problems with this branch

it would be better to don't create a new one, I can help you to figure out how to handle the issue (if you have still any issue on this branch), describe me what is the issue that you are facing and I'll look into it and provide you more hints on how to deal with it.

sonalprabhu · 2020-10-09T11:53:22Z

I cleared the issues with that.
Please re-review my PR

rpl · 2020-10-09T12:04:52Z

@sonalprabhu looks great! I just approved this version of the patch 🎉
Thanks a lot for taking care of this issue!

sonalprabhu · 2020-10-09T12:06:49Z

@sonalprabhu looks great! I just approved this version of the patch 🎉
Thanks a lot for taking care of this issue!

Thanks a lot for your detailed guidelines :)

caitmuenster · 2020-10-09T16:44:10Z

Thanks so much for the patch, @sonalprabhu! 🙌 Your contribution has been added to our recognition wiki.

Welcome onboard! We look forward to seeing you around the project.

sonalprabhu added 4 commits October 2, 2020 15:15

Remote debugging message shown repeatedly

7d20b04

Looped the error message with different color

42a2098

Removed trailing spaces

3b3dd58

Corrected the errors

702abc2

rpl requested changes Oct 2, 2020

View reviewed changes

src/extension-runners/firefox-android.js Outdated Show resolved Hide resolved

src/util/adb.js Outdated Show resolved Hide resolved

src/util/adb.js Outdated Show resolved Hide resolved

rpl changed the title ~~Fix: Improve visibility of message to enable "Remote Debugging via USB" with web-ext run -t firefox-android~~ fix: Improve visibility of message to enable "Remote Debugging via USB" with web-ext run -t firefox-android Oct 2, 2020

sonalprabhu and others added 21 commits October 2, 2020 17:26

Removed yellow color from warning

2ad9a9e

Added tests for enable Remote Debugging warning

5b785b0

Merge branch 'master' into loop-warning

39fabad

Remote debugging message shown repeatedly

0c95147

Looped the error message with different color

a3f8086

Removed trailing spaces

bafc597

Corrected the errors

e149055

Removed yellow color from warning

7673b0d

Added tests for enable Remote Debugging warning

a7fc6ba

chore(deps): update dependency eslint to v7.10.0 (mozilla#2033)

2cfcebe

Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency eslint-plugin-import to v2.22.1 (mozil…

15493db

…la#2034) Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency sinon to v9.1.0 (mozilla#2037)

aae0728

Co-authored-by: Renovate Bot <bot@renovateapp.com>

fix(deps): update dependency firefox-profile to v4 (mozilla#2030)

79f9a4b

Co-authored-by: Renovate Bot <bot@renovateapp.com>

fix(deps): update dependency addons-linter to v2.7.0 (mozilla#2032)

42ac6c8

Co-authored-by: Renovate Bot <bot@renovateapp.com>

fix(deps): update dependency open to v7.3.0 (mozilla#2004)

dd3bfa9

Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency git-rev-sync to v3 (mozilla#2013)

abbbb15

Co-authored-by: Renovate Bot <bot@renovateapp.com>

fix(deps): update dependency update-notifier to v5 (mozilla#2036)

e87f6d7

Co-authored-by: Renovate Bot <bot@renovateapp.com>

fix(deps): Switch from adbkit to @devicefarmer/adbkit npm package (mo…

cf0f9ea

…zilla#2039)

chore: Bump package version for release 5.2.0

cc18128

New test case

c8dc6dd

Update flow.js

dedb126

sonalprabhu requested a review from rpl October 8, 2020 17:00

rpl requested changes Oct 8, 2020

View reviewed changes

sonalprabhu and others added 4 commits October 9, 2020 14:37

Update test.adb.js

2e5c510

Update firefox-android.js

ae36534

Update test.firefox-android.js

2d5cc48

Merge branch 'master' into loop-warning

de16ffb

sonalprabhu and others added 4 commits October 9, 2020 16:01

Made all the requested changes

a0a8814

Merge branch 'loop-warning' of https://github.com/sonalprabhu/web-ext …

6821b86

…into loop-warning

Made the requested changes

aaa26f5

Update test.adb.js

3b5e1ca

rpl approved these changes Oct 9, 2020

View reviewed changes

rpl merged commit 4ab3043 into mozilla:master Oct 9, 2020

sonalprabhu deleted the loop-warning branch October 10, 2020 05:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve visibility of message to enable "Remote Debugging via USB" with web-ext run -t firefox-android #2038

fix: Improve visibility of message to enable "Remote Debugging via USB" with web-ext run -t firefox-android #2038

sonalprabhu commented Oct 2, 2020

coveralls commented Oct 2, 2020 •

edited

Loading

rpl commented Oct 2, 2020

rpl left a comment

rpl Oct 8, 2020

rpl Oct 8, 2020

rpl Oct 8, 2020

rpl Oct 8, 2020

rpl Oct 8, 2020

rpl commented Oct 9, 2020

rpl commented Oct 9, 2020 •

edited

Loading

sonalprabhu commented Oct 9, 2020 •

edited

Loading

rpl commented Oct 9, 2020

sonalprabhu commented Oct 9, 2020

caitmuenster commented Oct 9, 2020


		while (rdpUnixSockets.length === 0) {
		log.info(`\n${msg}\n`);

fix: Improve visibility of message to enable "Remote Debugging via USB" with web-ext run -t firefox-android #2038

fix: Improve visibility of message to enable "Remote Debugging via USB" with web-ext run -t firefox-android #2038

Conversation

sonalprabhu commented Oct 2, 2020

coveralls commented Oct 2, 2020 • edited Loading

rpl commented Oct 2, 2020

rpl left a comment

Choose a reason for hiding this comment

rpl Oct 8, 2020

Choose a reason for hiding this comment

rpl Oct 8, 2020

Choose a reason for hiding this comment

rpl Oct 8, 2020

Choose a reason for hiding this comment

rpl Oct 8, 2020

Choose a reason for hiding this comment

rpl Oct 8, 2020

Choose a reason for hiding this comment

rpl commented Oct 9, 2020

rpl commented Oct 9, 2020 • edited Loading

sonalprabhu commented Oct 9, 2020 • edited Loading

rpl commented Oct 9, 2020

sonalprabhu commented Oct 9, 2020

caitmuenster commented Oct 9, 2020

coveralls commented Oct 2, 2020 •

edited

Loading

rpl commented Oct 9, 2020 •

edited

Loading

sonalprabhu commented Oct 9, 2020 •

edited

Loading