feat: export the adb utilities to be used in third party nodejs scripts

[ankushduacodes]

attempts to resolve #1999

[ankushduacodes]

@rpl Am I seem to be on the correct path here?

Also I do have some doubts related to your recent comment
Doubt:

1. I was trying to make {adbBin: string} object as optional param but could not find a way to do it. I tried reading docs for optional params in flowtype but could resolve the issue. I tried to do it another way ie. to define a type:

export type adbParam{| 
    adbBin?: string 
|}

but with this I was having to call the function with an empty object something like: webExt.util.listADBDevices({}).

2. util.listADBFirefoxAPKs({adbBin: string}): Promise<Array>: same parameters of the previous one, but returns a Promise that resolves to an array of string that represents the apk packages identified as Firefox/GeckoView-based browser

Shouldn't this function also take a deviceId of which GeckoView packages we may like to see?

[ankushduacodes]

@rpl Just a friendly reminder for the comment above. Looking to get some insight :)

[rpl]

Besides the comments below (the main one is about making sure to load the adb module as lazily as possible), we may also start to think to the testing coverage.

You may take a look to the test that is already covering importing web-ext as a library:

• tests/functional/test.lib.imports.js
• tests/fixtures/require-as-cjs/test-require.js
• tests/fixtures/import-as-esm/test-import.mjs

As usually, feel free to ask me more questions once you have some questions or doubts about what to test and what strategy to use.

[ankushduacodes]

@rpl I have added some basic tests to check if functions are being exposed correctly. How are tests looking so far?

Few questions:

1. There's a lot of code duplication in test-import.mjs and test-require.js. I was thinking of defining a common function that will receive a top-level object webExt and tests those asserts. What do you think? If you do think its a good idea, then where should I define that function? in a new file?
2. Could please point me to direction of what else needs to be tested and where? I know we should also test lazy loading of adb but where and how?

Thank you!...

[rpl]

@rpl I have added some basic tests to check if functions are being exposed correctly. How are tests looking so far?

Few questions:

1. There's a lot of code duplication in [test-import.mjs](https://github.com/ankushduacodes/web-ext/blob/export_adb_utilities/tests/fixtures/import-as-esm/test-import.mjs) and [test-require.js](https://github.com/ankushduacodes/web-ext/blob/export_adb_utilities/tests/fixtures/require-as-cjs/test-require.js). I was thinking of defining a common function that will receive a top-level object `webExt` and tests those asserts. What do you think? If you do think its a good idea, then where should I define that function? in a new file?

@ankushduacodes Yeah, this sounds like a good idea, we could just add an helper function in a separate module file and load it in both to avoid repeating the same assertion in both the test files.
I think that it may be reasonable to move both tests/fixtures/import-as-esm and tests/fixtures/require-as-cjs under the same directory (e.g. one named tests/fixtures/webext-as-library or something like that) and then move both test-import.mjs and test-require.js under the same directory, and put the helper in a new helpers.js file in that same directory.

2. Could please point me to direction of what else needs to be tested and where? I know we should also test lazy loading of adb but where and how?

I think that one possible way to explicitly test that the adb module is lazily loaded may be to use the require.cache property that nodejs does provide. That cache property is an object that has the loaded module paths as keys.

You may try it interactively by running npm run build with your changes applied and then spawn a node repl from the web-ext project root:

$ node
Welcome to Node.js v12.16.3.
Type ".help" for more information.
> Object.keys(require.cache)
[]
> require('./dist/web-ext')
...
> Object.keys(require.cache)
[
  '/.../web-ext/dist/web-ext.js',
  '/.../web-ext/node_modules/source-map-support/source-map-support.js',
  ...
]

Using this strategy we may do inside the test helper that you are going to create (the one that will be used by both test-import.mjs and test-require.js) something like the following right after requiring/importing 'web-ext':

• before accessing webExt.util.adb, assert that Object.keys(require.cache).filter(filePath => filePath.includes('@devicefarmer/adbkit')) is 0 (we didn't accessed util.adb and so we expect that the "@devicefarmer/adbkit" dependencies should not have been loaded yet)
• after accessing webExt.util.adb, assert that Object.keys(require.cache).filter(filePath => filePath.includes('@devicefarmer/adbkit')) is greater than 0 (because at that point we expect that the ""@devicefarmer/adbkit" dependencies has been loaded)

This last assertion will also make us sure that if we will ever change the dependency the test will fail and that will remind us to update the test with the new dependency (it will be basically a safety net to be sure that the test doesn't turn itself into an "always passing even if broken" kind of test without being noticed ;-)).

Let me know if this pointers were helpful or if you have any further doubts or questions.

[codecov-io]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (f1ed131) to head (315c0f7).
⚠️ Report is 1085 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2116   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           32        32           
  Lines         1567      1573    +6     
=========================================
+ Hits          1567      1573    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ankushduacodes]

@rpl I have a doubt about imports:
When I tried importing helper.js the following way:

import _ from './helpers';

It gave me an error and the test of importing web-ext as ESM module failed
But when I used:

import _ from './helpers.js';

the test passes.

Why is that so?

Edit: I did some reading on it. So to my understanding first import statement fails because it looks for helpers directory instead of the file. Please correct me if I am wrong.

[ankushduacodes]

2. Could please point me to direction of what else needs to be tested and where? I know we should also test lazy loading of adb but where and how?

I think that one possible way to explicitly test that the adb module is lazily loaded may be to use the require.cache property that nodejs does provide. That cache property is an object that has the loaded module paths as keys.

You may try it interactively by running npm run build with your changes applied and then spawn a node repl from the web-ext project root:

$ node
Welcome to Node.js v12.16.3.
Type ".help" for more information.
> Object.keys(require.cache)
[]
> require('./dist/web-ext')
...
> Object.keys(require.cache)
[
  '/.../web-ext/dist/web-ext.js',
  '/.../web-ext/node_modules/source-map-support/source-map-support.js',
  ...
]

Using this strategy we may do inside the test helper that you are going to create (the one that will be used by both test-import.mjs and test-require.js) something like the following right after requiring/importing 'web-ext':

• before accessing webExt.util.adb, assert that Object.keys(require.cache).filter(filePath => filePath.includes('@devicefarmer/adbkit')) is 0 (we didn't accessed util.adb and so we expect that the "@devicefarmer/adbkit" dependencies should not have been loaded yet)
• after accessing webExt.util.adb, assert that Object.keys(require.cache).filter(filePath => filePath.includes('@devicefarmer/adbkit')) is greater than 0 (because at that point we expect that the ""@devicefarmer/adbkit" dependencies has been loaded)

This last assertion will also make us sure that if we will ever change the dependency the test will fail and that will remind us to update the test with the new dependency (it will be basically a safety net to be sure that the test doesn't turn itself into an "always passing even if broken" kind of test without being noticed ;-)).

Let me know if this pointers were helpful or if you have any further doubts or questions.

@rpl I was looking into implementing an interactive node repl to execute above commands and the check for then assert for the statements above. I did look into spawning a child process and processing the data something like following:

cp.execFileSync(npm, ['run', 'build']).toString();
  const childProcess = cp.spawn('node', ['-i']);

  childProcess.stdout.setEncoding('utf8')
  childProcess.stdout.on("data", function(data) {
      // process/assert here
  });


  childProcess.stdin.write("Object.keys(require.cache)\n");
  childProcess.stdin.write("const webExt = require('./dist/web-ext')\n");
  childProcess.stdin.write("Object.keys(require.cache).filter(filePath => filePath.includes('@devicefarmer/adbkit'))\n");
  childProcess.stdin.write("webExt.util.adb\n");
  childProcess.stdin.write("Object.keys(require.cache).filter(filePath => filePath.includes('@devicefarmer/adbkit'))\n");
  childProcess.stdin.end()

But the issue I am facing, in this case, is that event is triggered every time new data is sent to stdout even if it consists of > or a new line or repl welcome message. And I am not able to run asserts on the data being sent as I don't know what data is being sent.

I hope I am making some sense here.

[rpl]

@ankushduacodes follows a new round of review comments (you may also remove the Draft from this PR, it is on the right track, and also actively developed and reviewed) with some more details about how to cover with the functional test the expected lazy loading behavior of the 'adb.js' module (plus some other nits and a comment related to some more test coverage needed in this pull request).

Let me know how it goes (and if you have any further doubts or questions).

[rpl]

@ankushduacodes thanks a lot for the work on this enhancement (and for sticking with me during the multiple discussions about implementation strategies, multiple review passes and CI failures to investigate).

There are 3 small nits described below that I would like to be applied in this patch, but feel free to let me know if you already moved to work on something else and you can't apply them, I'll be happy to make those small changes and merge this (obviously keeping the author metadata as is ;-))

[ankushduacodes]

On it. Thank you.

(PS: If there is anything left, do let me know. I don't delete branches until after a PR is merged. :))

[rpl]

On it. Thank you.

(PS: If there is anything left, do let me know. I don't delete branches until after a PR is merged. :))

Thanks @ankushduacodes for handling the last few nits as well, I just merged this on the main branch! 🚀 \o/

[ankushduacodes]

Thank you @rpl for the guidance along the way and patiently explaining my (sometimes silly) doubts. I am learning so much. Many more to go.
cheers 🚀 🎉