feat: Introduce webext run --devtools to open DevTools for the installed add-on right away. by ochameau · Pull Request #2488 · mozilla/web-ext

ochameau · 2022-08-26T09:06:36Z

This depends on bug 1787409 and aims at addressing issue #759.

rpl

@ochameau thank you so much for looking into the improvements to the webextensions debugging, I just tried this patch locally combined with (most of) the changes from Bug 1787409 (and the other patch in that stack) and it definitely provides a nicer developer experience.

It seems also (at least if I'm not mistaken) that on older Firefox versions where the changes from Bug 1787409 will not be available the additional openDevTools property in the installTemporaryAddon RDP request is just ignored, and so it looks that we will not need to do any runtime detection and fallback, which is also great.

Follows a couple of small tweaks to let the PR to run all tests in the CI job (fixing linting and flow error, and an unrelated test failure due to the change to the method signature that the sinon stub wasn't expecting).

Would you mind to also remove the package-lock.json change? that is unrelated (and pretty big because running npm install with an older npm version rewrite the entire lock file to lockfileVersion 1).

You should be able to avoid the automatic rewite of the lock file by running npm ci instead of npm install.

ochameau · 2022-08-30T07:43:20Z

It seems also (at least if I'm not mistaken) that on older Firefox versions where the changes from Bug 1787409 will not be available the additional openDevTools property in the installTemporaryAddon RDP request is just ignored, and so it looks that we will not need to do any runtime detection and fallback, which is also great.

Yes I confirm, no backward compat is needed, unless we want to warn when using --devtools on an unsupported version.

Thanks for all the suggestion to fix all ci issues \o/

…led add-on right away. This depends on bug 1787409.

ochameau · 2022-08-30T07:45:04Z

I only started asking for review on https://bugzilla.mozilla.org/show_bug.cgi?id=1787409
So no rush in reviewing/landing this.

ochameau · 2022-09-01T15:44:57Z

I added a new changeset to promote this new command line argument and also mention https://extensionworkshop.com/documentation/develop/debugging/

Document the new command line argument introduced in mozilla/web-ext#2488

ochameau · 2022-09-01T15:51:14Z

I'm also trying to document this new command line argument over there.

ochameau · 2022-09-06T18:16:55Z

I pushed a change to fix lint issues, but jobs are still failing whereas yarn test works locally.

rpl · 2022-09-06T19:11:42Z

I pushed a change to fix lint issues, but jobs are still failing whereas yarn test works locally.

@ochameau sorry for not having got back to this sooner, I noticed it and I was meant to comment but didn't got back to this quickly enough (or at least not as quickly as you ;-))

Those failures are coming from the function, which are the ones that can be also run locally using npm run test-functional, the reason for the failure (and related fix) is actually trivial:

the web-ext run functional test is currently using a nodejs script that mocks Firefox and it doesn't currently expect the new option and so it fails, the reason is not immediately visible in the failure message but it is part of the failure logs emitted by the tests/functional/fake-firefox-binary.js script here where it says Fake Firefox received an unexpected message

The following one line patch should fix that remaining CI job failure:

diff --git a/tests/functional/fake-firefox-binary.js b/tests/functional/fake-firefox-binary.js
index 2f86902..00ffb3f 100755
--- a/tests/functional/fake-firefox-binary.js
+++ b/tests/functional/fake-firefox-binary.js
@@ -9,6 +9,7 @@ const REQUEST_INSTALL_ADDON = {
   to: 'fakeAddonsActor',
   type: 'installTemporaryAddon',
   addonPath: process.env.addonPath,
+  openDevTools: false, // Introduced in Firefox 106 (Bug 1787409 / Bug 1789245)
 };
 const REPLY_INSTALL_ADDON = {
   from: 'fakeAddonsActor',

ochameau · 2022-09-06T20:06:26Z

Thanks for the investigate. I hope jobs will be green now.

codecov · 2022-09-06T20:12:07Z

Codecov Report

Merging #2488 (6f58053) into master (c932666) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2488   +/-   ##
=======================================
  Coverage   99.52%   99.52%           
=======================================
  Files          32       32           
  Lines        1677     1681    +4     
=======================================
+ Hits         1669     1673    +4     
  Misses          8        8

Impacted Files	Coverage Δ
src/cmd/run.js	`100.00% <ø> (ø)`
src/extension-runners/firefox-desktop.js	`100.00% <ø> (ø)`
src/firefox/remote.js	`100.00% <ø> (ø)`
src/program.js	`98.55% <ø> (ø)`
src/firefox/index.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rpl

@ochameau looks pretty great, I have just one more thoughts described in the inline review comment below (I'm basically wondering if we really need to condition those logs, if we think it is not necessary we can revert a couple of changes related to propagating verbose and avoid the codecov failure as a side effect).

rpl · 2022-09-07T10:27:01Z

+  if (!verbose && !devtools) {
+    log.info('Use --verbose or --devtools to see logging');
+  }
+  if (devtools) {
+    log.info('More info about WebExtension debugging:');
+    log.info('https://extensionworkshop.com/documentation/develop/debugging/');


@ochameau I'm thinking that we may as well always log this couple of messages, instead of condition them on the verbose and devtools flags.

wdyt?

As a side-effect, that may also make codecov to don't complain about these logs emitted on devtools === true not being covered in tests (which are not a big deal on their own, given that its just two informative logs I wouldn't definitely block on that codecov coverage failure).

TBH, I'm not working enough with this tooling to have a strong opinion.

So I would prefer to defer to you/your team judgement.
I'm fine removing, or adding proper coverage.

ok, let's remove the verbose flag propagation and keep that log unconditioned, while condition the new one to the devtools flag.

Let's also change WebExtension to WebExtensions in the new logged message and let's add something like this to test.firefox.js to explicitly cover it and make codecov happy again:

import { consoleStream, // instance is imported to inspect logged messages } from '../../../src/util/logger.js'; ... it('logs link to debugging docs', async () => { const runner = createFakeFxRunner(); consoleStream.flushCapturedLogs(); consoleStream.startCapturing(); const expectedMessage = 'More info about WebExtensions debugging:'; const expectedURLToDocs = 'https://extensionworkshop.com/documentation/develop/debugging/'; await runFirefox({fxRunner: runner, devtools: false}); assert.notOk(consoleStream.capturedMessages.find( (msg) => msg.includes(expectedMessage) )); assert.notOk(consoleStream.capturedMessages.find( (msg) => msg.includes(expectedURLToDocs) )); consoleStream.flushCapturedLogs(); await runFirefox({fxRunner: runner, devtools: true}); const foundMessage = consoleStream.capturedMessages.find( (msg) => msg.includes(expectedMessage) ); const foundDocURL = consoleStream.capturedMessages.find( (msg) => msg.includes(expectedURLToDocs) ); // Expect the logs to be found. assert.ok(foundMessage); assert.ok(foundDocURL); // Expected to be emitted as info level logs. assert.ok(foundMessage?.includes('[info]')); assert.ok(foundDocURL?.includes('[info]')); });

rpl · 2022-09-08T11:53:43Z

+  if (!verbose && !devtools) {
+    log.info('Use --verbose or --devtools to see logging');
+  }
+  if (devtools) {
+    log.info('More info about WebExtension debugging:');
+    log.info('https://extensionworkshop.com/documentation/develop/debugging/');


ok, let's remove the verbose flag propagation and keep that log unconditioned, while condition the new one to the devtools flag.

Let's also change WebExtension to WebExtensions in the new logged message and let's add something like this to test.firefox.js to explicitly cover it and make codecov happy again:

import { consoleStream, // instance is imported to inspect logged messages } from '../../../src/util/logger.js'; ... it('logs link to debugging docs', async () => { const runner = createFakeFxRunner(); consoleStream.flushCapturedLogs(); consoleStream.startCapturing(); const expectedMessage = 'More info about WebExtensions debugging:'; const expectedURLToDocs = 'https://extensionworkshop.com/documentation/develop/debugging/'; await runFirefox({fxRunner: runner, devtools: false}); assert.notOk(consoleStream.capturedMessages.find( (msg) => msg.includes(expectedMessage) )); assert.notOk(consoleStream.capturedMessages.find( (msg) => msg.includes(expectedURLToDocs) )); consoleStream.flushCapturedLogs(); await runFirefox({fxRunner: runner, devtools: true}); const foundMessage = consoleStream.capturedMessages.find( (msg) => msg.includes(expectedMessage) ); const foundDocURL = consoleStream.capturedMessages.find( (msg) => msg.includes(expectedURLToDocs) ); // Expect the logs to be found. assert.ok(foundMessage); assert.ok(foundDocURL); // Expected to be emitted as info level logs. assert.ok(foundMessage?.includes('[info]')); assert.ok(foundDocURL?.includes('[info]')); });

…g online documentation

ochameau · 2022-09-08T13:32:14Z

Thanks a lot for the test snippet!

rpl

lgtm

@ochameau thank you so much for the work you have done on both Firefox and web-ext side for introducing this new option! ❤️

willdurand · 2022-09-12T11:47:30Z

I am going to land this PR to avoid future conflicts.

Document the new command line argument introduced in mozilla/web-ext#2488

rpl requested changes Aug 26, 2022

View reviewed changes

Comment thread src/program.js Outdated

Comment thread src/program.js Outdated

Comment thread src/firefox/remote.js Outdated

feat: Introduce webext run --devtools to open DevTools for the instal…

94989fa

…led add-on right away. This depends on bug 1787409.

ochameau force-pushed the devtoolsArgument branch from 2de9030 to 94989fa Compare August 30, 2022 07:44

ochameau added a commit to ochameau/extension-workshop that referenced this pull request Sep 1, 2022

Document web-ext run --devtools command line argument

ea1197f

Document the new command line argument introduced in mozilla/web-ext#2488

ochameau mentioned this pull request Sep 1, 2022

Document web-ext run --devtools command line argument mozilla/extension-workshop#1398

Merged

ochameau requested a review from rpl September 6, 2022 17:30

ochameau force-pushed the devtoolsArgument branch from 81a0f49 to 1f14786 Compare September 6, 2022 18:03

ochameau force-pushed the devtoolsArgument branch from 1f14786 to 87e5423 Compare September 6, 2022 20:05

rpl reviewed Sep 7, 2022

View reviewed changes

rpl requested changes Sep 8, 2022

View reviewed changes

Promote the new --devtools command line argument and link to debuggin…

6f58053

…g online documentation

ochameau force-pushed the devtoolsArgument branch from 87e5423 to 6f58053 Compare September 8, 2022 13:31

ochameau requested a review from rpl September 8, 2022 13:32

rpl approved these changes Sep 8, 2022

View reviewed changes

willdurand merged commit c8f5ae4 into mozilla:master Sep 12, 2022

willdurand mentioned this pull request Sep 12, 2022

web-ext run should allow for starting with debugger enabled. #759

Closed

ochameau added a commit to ochameau/extension-workshop that referenced this pull request Sep 12, 2022

Document web-ext run --devtools command line argument

d986b36

Document the new command line argument introduced in mozilla/web-ext#2488

Conversation

ochameau commented Aug 26, 2022

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ochameau commented Aug 30, 2022

Uh oh!

ochameau commented Aug 30, 2022

Uh oh!

ochameau commented Sep 1, 2022

Uh oh!

ochameau commented Sep 1, 2022

Uh oh!

ochameau commented Sep 6, 2022

Uh oh!

rpl commented Sep 6, 2022

Uh oh!

ochameau commented Sep 6, 2022

Uh oh!

codecov Bot commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

rpl Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

ochameau Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

rpl Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

rpl Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

ochameau commented Sep 8, 2022

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

willdurand commented Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Sep 6, 2022 •

edited

Loading