Incorrectly written test cases masking test failure

[kitsonk]

Bug

I noticed an unhandled promise rejection somewhere in our test suite, which made me look a bit deeper. Several of our test cases are written wrong and one of them is actually failing, but because the way it is written is causing it to look like it is passing.

The unhandled promise occurs in this block of code:

return new Promise((resolve, reject) => {
    resolver({ resource: '/test/module.js' }, () => {
        resolve();
        assert.isTrue((<any> plugin.resolve).called);
        assert.isTrue((<any> plugin.createModules).calledWith([ { context: '/test', request: './module' } ]));
    });
});

First, once a promise is resolved, no other tracking by Intern occurs. Second, throws in a Promise callback do not automatically reject the promise. If we wanted this code to actually fail when the assertions throw, it would need to look like this:

return new Promise((resolve, reject) => {
    resolver({ resource: '/test/module.js' }, () => {
        try {
            assert.isTrue((<any> plugin.resolve).called);
            assert.isTrue((<any> plugin.createModules).calledWith([ { context: '/test', request: './module' } ]));
        }
        catch (e) {
            reject(e);
        }
        resolve();
    });
});

While this one appears to be the only one failing, there are several others that are written in a way they won't actually fail irrespective of the assertions succeeding or failing.

This also means that there is either a defect with the test case or the code, because if changed, you get the following error:

× inject-modules - "resolver" factory plugin - should generate data for matching issuers (0.003s)
AssertionError: expected false to be true
  at Assertion.<anonymous>  <node_modules/intern/browser_modules/chai/chai.js:567:10>
  at Assertion.addProperty  <node_modules/intern/browser_modules/chai/chai.js:4240:29>
  at Function.assert.isTrue  <node_modules/intern/browser_modules/chai/chai.js:2483:31>
  at resolver  <tests/unit/plugins/InjectModulesPlugin.ts:379:13>
  at resolve.then.then  </Users/kitsonk/github/cli-build/src/plugins/InjectModulesPlugin.ts:167:9>
  at runMicrotasksCallback  <internal/process/next_tick.js:64:5>
  at _combinedTickCallback  <internal/process/next_tick.js:73:7>
  at process._tickCallback  <internal/process/next_tick.js:104:9>

[kitsonk]

@matt-gadd pointed out that throwing in resolver callbacks does implicitly reject with the error. So all that needs to change is only resolve() after the assertions, because resolved promises cannot be rejected:

return new Promise((resolve, reject) => {
    resolver({ resource: '/test/module.js' }, () => {
        assert.isTrue((<any> plugin.resolve).called);
        assert.isTrue((<any> plugin.createModules).calledWith([ { context: '/test', request: './module' } ]));
        resolve();
    });
});

[mwistrand]

In this particular case, the try/catch block is still necessary, since without it, the error logged to the console is a timeout error, not the test case error.

[kitsonk]

This has an outstanding approved PR that resolves this issue. We should focus on merging it.