Suggested fix on problem with failed tests and grunt-contrib-watch

[christianalfoni]

First, fantastic job on this :-)

I had issues using grunt-buster with grunt-contrib-watch. I had no problems running them together, but when a test fails the "deferred.reject()" call causes the "sequence" in the buster.js task file to run the reject callback, not passing the two server variables needed to stop grunt-server and phantomJS. This causes the "stop()" function in buster.js to actually break since it tries to resolve "results[0]" and "results[1]" of an argument that is undefined.

All of this ends up stopping the grunt task whenever a test fails, not continuing with the watch task to rerun everything when you have fixed the test.

I tried a couple of different solutions, but ended up with the thought that if a test fails it does not mean that the sequence failed. It should still close down the servers.

Hopefully I got this right :-)

[myme]

Thanks for the feedback and the pull request. I agree with your assumption that both buster-server and phantomjs should be properly terminated even if there is a test failure. I am also able to reproduce your reported issue. I also appreciate the fact that you supply a potential fix to the issue. Unfortunately, it does not resolve the issue in a satisfactory manner, in my opinion. The reason being that the buster task relies on the promise resolution from the buster-test task in order to determine if the tests passed or failed, and thus reporting a proper success or failure back to Grunt (which in turn exits with a proper exit code, etc).

The reason for the bug is exactly what you state, that sequence does not pass in the proper arguments to the stop function. This was due to an incorrect assumption on my part, which was that sequence would reject the promise with all successful tasks up until the point of failure, then include the value of the rejected promise. Instead it only rejects with the first failed task.

I'll submit a separate pull request trying to address this with #26.

[christianalfoni]

Hi again, I had a go at it myself. I am not very experienced with promises, which often ends up in taking a very direct approach :-) Just hoped you had some input on if it is a crappy solution or if it would have worked just fine ;-)

    // The serverPromises is needed to shut them down
    var serverPromises = [];
    var createBusterServerPromise = function () {
        if (runServer) {
            return cmd.runBusterServer(config.getArguments('server', configData));
        }
        return null;
    };
    var createPhantomjsPromise = function () {
        if (runPhantomjs) {
            return cmd.runPhantomjs(config.getArguments('phantomjs', configData));
        }
        return null;
    };
    var createBusterTestsPromise = function () {
        if (runTests) {
            return cmd.runBusterTest(config.getArguments('test', configData));
        }
        return null;
    };

    // Add phantomJs value and stop based on tests being resolved or not
    var onPhantomjsReady = function (result) {
        serverPromises.push(result);
        when(createBusterTestsPromise(), function () {
            stop(null, serverPromises);
        }, function () {
            stop(false, serverPromises);
        });
    }
    // Add buster-server value and start phantomJs
    var onBusterServerReady = function (result) {
        serverPromises.push(result);
        when(createPhantomjsPromise(), onPhantomjsReady, onPhantomjsReady);
    }
    // Start buster-server
    when(createBusterServerPromise(), onBusterServerReady, onBusterServerReady);

[myme]

I haven't looked too closely at your implementation, but I think it would work. The thing I didn't like about the original implementation we had in tasks/buster.js, was the readability. I wanted to preserve the semantics with the new implementation using sequence. Your code suffers from some of the same readability issues, mostly due to the fact that the execution flow is going in somewhat the opposite direction of the code. The other issue being that it does not distinguish between a resolved and rejected server task. Besides that, I think it does pretty much the same.

A challenge with asynchronous code, is writing it so that it maintains its readability when compared to a hypothetical synchronous approach. By using higher-order constructs like sequence, it's possible to abstract away the cruft required to maintain dependencies between several asynchronous tasks. This can be difficult at first, but like with everything else, all it takes is exposure and practice. Hopefully that results in code which is easier to read and reason about, which leads to fewer bugs in the future. So I leave it up to you to determine if the current code is in fact more readable than what was there before ;-)

[christianalfoni]

Hehe, thanks for a very thorough answer, I should definitely read read up on promises. Btw, thanks for the quick fix :-)

I’m having a presentation about Buster.JS tomorrow at work and I’ve asked my boss to put it online. It took me a bit of time to research all the possibilities with grunt, amd, testbed, sinon, coverage etc., mostly because there is a lack of “get going” tutorials out there. Hopefully the summary I’m presenting will be a contribution to that! ;-)

Keep up the good work!

From: Martin Øinæs Myrseth notifications@github.com<mailto:notifications@github.com> Reply-To: busterjs/grunt-buster reply@reply.github.com<mailto:reply@reply.github.com> Date: Monday 4 November 2013 14:52 To: busterjs/grunt-buster grunt-buster@noreply.github.com<mailto:grunt-buster@noreply.github.com> Cc: Christian Jørgensen christian.jorgensen@marcello.no<mailto:christian.jorgensen@marcello.no> Subject: Re: [grunt-buster] Suggested fix on problem with failed tests and grunt-contrib-watch (#25)

I haven't looked too closely at your implementation, but I think it would work. The thing I didn't like about the original implementation we had in tasks/buster.js, was the readability. I wanted to preserve the semantics with the new implementation using sequence. Your code suffers from some of the same readability issues, mostly due to the fact that the execution flow is going in somewhat the opposite direction of the code. The other issue being that it does not distinguish between a resolved and rejected server task. Besides that, I think it does pretty much the same.

A challenge with asynchronous code, is writing it so that it maintains its readability when compared to a hypothetical synchronous approach. By using higher-order constructs like sequence, it's possible to abstract away the cruft required to maintain dependencies between several asynchronous tasks. This can be difficult at first, but like with everything else, all it takes is exposure and practice. Hopefully that results in code which is easier to read and reason about, which leads to fewer bugs in the future. So I leave it up to you to determine if the current code is in fact more readable than what was there before ;-)

— Reply to this email directly or view it on GitHubhttps://github.com/busterjs/grunt-buster/pull/25#issuecomment-27685817.