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

Merging #2116 (9616682) into master (f1ed131) will not change coverage.
The diff coverage is 100.00%.

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

Impacted Files	Coverage Δ
src/main.js	100.00% <100.00%> (ø)
src/util/adb.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1ed131...9616682. Read the comment docs.

[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).

[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 🚀 🎉