wip: footgun protective boots.

[gustavnikolaj]

I just had a case where a failing test with http mocked out was causing a bunch of later mitm tests to fail miserably because mitm didn't get around to cleaning up.

@papandreou told me that I was suffering from the classic "uncaught exception in async flow breaking promises"-problem that unexpected also faces.

This is a really naive fix that obviously needs improvement, but it saved the day in my test suite :-)

[coveralls]

Coverage decreased (-0.3%) to 95.336% when pulling e25ab0f on feature/footGun into cdffc16 on master.

[coveralls]

Coverage decreased (-0.3%) to 95.336% when pulling e25ab0f on feature/footGun into cdffc16 on master.

[alexjeffburke]

@gustavnikolaj After our conversation I've now read the PR and completely get what you're trying to address - I've definitely seen these symptoms before, but it was usually when I was making changes to the error reporting logic and did something wrong.

The thing that makes me rather uneasy is just that this really shouldn't happen - the intention of all the error paths in -mitm we catch both our own issues and any occurring in the delegated assertion, correctly do a reject() and then the finally block must always run. Agree this is an ultimate mitigation, but I'd much rather track down how an exception can escape and cause this. Plus, this is rather closely tied to the existence of something like an afterEach() mechanism - I know we already do something cheeky like this with after() in the injection assertion but I think we should try to avoid it.

All that being said, it may well be there is no other way to address this - but if there is any chance of getting hold of a request that generates this it would be really great, even if it's just to become aware of the circumstances where this isn't avoidable.

[papandreou]

This is missing some context -- I'm sorry I had to leave our general unexpected-mitm meeting so abruptly :)

The problem @gustavnikolaj ran into was something like this:

var expect = require('unexpected').clone().use(require('unexpected-mitm'));

it('should foo', function () {
    return expect(function () {
        setTimeout(function () {
            throw new Error('Hot damn');
        });
    }, 'with http mocked out', [], 'to call the callback');
});

it('should bar', function () {
    // Oh no, unexpected-mitm is still hooked up here, so this test
    // will fail with "unexpected-mitm: Saw unexpected requests":
    return expect.promise(function (run) {
        var req = require('http').request('http://google.com/');
        req.end();
        req.on('response', run(function (response) {
            expect(response.statusCode, 'to equal', 302);
        }));
    });
});

Essentially, when an uncaught exception is caught by mocha, unexpected-mitm doesn't clean up, and it bleeds into the remaining tests. This happens more often than you might think when testing callback-oriented code (see unexpectedjs/unexpected#212).

Since Unexpected itself already tries to register an afterEach handler, it could make sense to expose some kind of "emergency cleanup hook" there that also includes the ability to fail the test instead of putting it in each plugin that needs this soft of thing (at least unexpected-{fs,mitm,mxhr}).

That could also pave the way for a solution to unexpectedjs/unexpected#292 -- what if we could declare the mocked out traffic for the entire it block using something like:

it('should foo', function () {
    expect([
        { request: 'GET http://www.google.com/', response: 200 },
        { request: 'GET http://www.google.com/foo', response: 200 }
    ], 'to be the http traffic happening during this test');

    return expect('GET http://www.google.com/', 'to yield response', 200)
        .then(function () {...});
});

// CC: @sunesimonsen

[alexjeffburke]

No probs! Mostly had my head around it but seeing some code in person is really useful. Does the setTimeout() mean that no amount of try-catch in anything unexpected side can get hold of the exception? That's a pretty nasty crack in the universe where promises are involved if so.

As for that second example, it's funny you say that because I had an API like that prototyped for -events. May not have explained it well at the time, but it's what I meant around 'capturing' the events and asserting later. May even still have the branch, will look as I had some of the issues written up at one point.

[papandreou]

setTimeout() mean that no amount of try-catch in anything unexpected side can get hold of the exception?

Yes. The same will be the case here:

it('should quux', function () {
    expect(function (cb) {
        doSomethingAsync(function (err, result) {
            expect(err, 'to be null');
        });
    }, 'to call the callback');
});

Only going process.on('uncaughtException', function (err) {...}); or window.onerror = function (err) {...}); will reveal it.

[papandreou]

but it's what I meant around 'capturing' the events and asserting later

Ah, I remember you asking about whether it was possible to learn mocha's timeout for the current test? Would the ability to run code after the test (afterEach) give you the needed power as well?

[alexjeffburke]

@papandreou yeah, all that stuff was intertwined :p So the obvious thing about event emitters is that you have to attach to them before any events occur. That basically translates to you wanting to call .on() up front but have the dependent code execute on the next tick. The major problem with wrapping that up with an unexpected assertion is that the kind of code flow I wanted was to have the assertion on produced events after the code being tested ran, so something like:

expect(function (cb) {
    // some pretty important code... cb();
}, 'to call the callback without error').then(function () {
    expect(theEmitter, 'has the event', ['something']);
});

but that hypothetical 'has the event' would have attached too late if it was naiively written. So the experimental solution I came up with was a pre-attachment, like expect(theEmitter, 'to have some events'); and then later you assert on that. This is what kind of brought the idea of context back up for me at the time - but you totally reminded of it because it's very similar to the 'to be the http traffic happening during this test' assertion you describe.

I got a version of it working (between somewhat & incredibly hacky with what's currently available), but the feeling I was left with is that it would hugely benefit from proper support in core. Apologies for the wall of text, but I'm hoping it's useful and consider it me taking an opportunity because it seems we're at the cusp of a very similar idea that I suspect may provide some pretty interesting abilities.