Remove all flow adapters, use flow in all tests

[kumar303]

Fixes #87

Specifically:

• removes all adapter files that were needed to get flow coverage
• adds flow coverage to all test files
• moves the bin/web-ext smoke test to npm test so it doesn't slow down npm run develop
• adds missing test coverage to errors.js

[kumar303]

@rpl could you review? It's mostly refactoring so sorry for the noise.

For some reason I don't see a link to coveralls in the PR checks, not sure why though. I see the coveralls command in the build output.

[kumar303]

I'm tempted to kill this test because it's slow and sometimes causes a timeout on Travis. It seems useful though because it will cover a catastrophic error in the bin/web-ext file and maybe also program.main() (although that has some coverage).

[rpl]

If we don't need its contribute on the testing code coverage, how about opt to test it outside of the mocha tests, as a shell command?

e.g. by adding to .travis.yml something like:

script: COVERAGE=y npm test && (cd ..; $TRAVIS_BUILD_DIR/bin/web-ext --help | grep -G "Usage: .*web-ext")

[kumar303]

oh yeah! I like that idea

[kumar303]

I moved it to npm test which runs during CI. The check will not be run in npm run develop which makes the test loop a lot faster.

[rpl]

@kumar303 +1 that's even better

[rpl]

Currently when this error is thrown (it shouldn't, but if it happens) the test case which use this helpers (tests/test-cmd/test-build.js) fails to detect the error and it fails for timeout reasons instead of showing the error throwed in the failure logs.

I propose to tweak the test case into something like the following diff:

diff --git a/tests/test-cmd/test.build.js b/tests/test-cmd/test.build.js
index ef19418..d5eff39 100644
--- a/tests/test-cmd/test.build.js
+++ b/tests/test-cmd/test.build.js
@@ -28,21 +28,27 @@ describe('build', () => {
                          /minimal_extension-1\.0\.xpi$/);
             return buildResult.extensionPath;
           })
           .then((extensionPath) => zipFile.open(extensionPath))
           .then(() => {
             var fileNames = [];
-            return new Promise((resolve) => {
+            return new Promise((resolve, reject) => {
               zipFile.readEach((entry) => {
                 fileNames.push(entry.fileName);
               })
               .then(() => {
                 resolve(fileNames);
+              })
+              .catch((error) => {
+                reject(error);
               });
             });
           })
           .then((fileNames) => {
             assert.deepEqual(fileNames, ['manifest.json']);
           })
+          .catch((error) => {
+            assert.ifError(error);
+          })
       );
     });

[kumar303]

Thanks for catching that

[kumar303]

(fixed)

[rpl]

@kumar303 this PR looks good

I just added a couple of notes/ideas above, about catching a potential failure in the build test cases and a proposal for moving the intermittent test case out of mocha.

NOTE: I noticed that this PR doesn't have coveralls in the runned PR checks, I wonder why.

[kumar303]

I noticed that this PR doesn't have coveralls

I can't figure out why either. Other more recent pull requests have coverage and I've even rebased this one a couple times. Maybe it is triggering a silent failure in coveralls?

[kumar303]

Well, huh. I can't see why coverage isn't getting published. I'm just going to merge it anyway because I can see coverage being reported on newer pull requests (even one that's based off of this branch).

Thanks for the review!