busterjs / buster

Abandoned - A powerful suite of automated test tools for JavaScript.
http://docs.busterjs.org
Other
448 stars 37 forks source link

Async test cases in browser, useful for AMD, RequireJS #15

Closed augustl closed 12 years ago

augustl commented 12 years ago

Buster currently can't facilitate these types of tests:

require(["my/file"], function () {
    buster.testCase("blah", {
        // ....
    });
});

The difficulty lies in knowing when all the buster.testCase functions have called. body.onload is not enough, due to the async require. We need to know when the test run has "finished" so we can end the session in buster-capture-server and exit the "buster test" process in the terminal, so we can't just wait 10 seconds and hope all the tests have loaded by then..

We need some sort of counter to let buster know before body.onload how many test cases that it expects to load. Suggestion:

buster.asyncTestCase(function (testCase) {
    require(["my/file"], function () {
        testCase("My test case", {
            // ....
        });
    });
});

With this model, buster knows at body.onload that there is one async test case that will be loaded in the future. It can wait for the passed "testCase" (which is just a wrapped buster.testCase) function to be called and know that if it isn't called it can timeout, and so on.

geddski commented 12 years ago

What would that look like in the BDD style?

magnars commented 12 years ago

I might be talking crazy here, but how about wrapping the require call? (ruby: alias_method_call, elisp: around advice) That way buster could keep a track of them without the extra syntax.

cjohansen commented 12 years ago

I also thought of wrapping the require function. But that would have to be a RequireJS extension to reduce the boiler-plate, and we might need to consider other loaders as well. We can do that, but we need a generic underlying mechanism that the extensions can use. I think the current autoRun: false + buster.run() is not sufficient.

I really like @augustl's suggestion, but I'd like to do it without a "new thing". In particular I'd like to play on the "a function with a callback makes the thing async" concept we already have for tests:

buster.testCase("I am a testcase", function (run) {
    require(["something"], function (something) {
        run({
            "here's the test": function () {

            }
        });
    });
});

I'm not crazy about the nesting here, but we could provide extensions for popular AMD loaders that reduce the boilerplate. You can also improve the situation manually by using named functions and variables... This model also extends nicely to the BDD stuff:

describe("Something", function (run) {
    require(["something"], function (something) {
        run(function () {
            it("does things", function () {});
        });
    });
});

The cool thing about this suggestion is that we can easily support it directly in the test runner. This means it will be easy to reuse the logic should someone want to create a new frontend (i.e. something that's not testCase nor describe). It also means relatively few changes to support them - only in the runner and testCase + describe.

What say ye?

magnars commented 12 years ago

Yes, that's nice. I like that it matches done. And with boilerplate-reducing extensions for popular script loaders, it's looking like an excellent solution.

augustl commented 12 years ago

+1 to buster.testCase("foo", function (run) { run(rootContextHere )});!

cjohansen commented 12 years ago

Ok, I'll fix it. If anyone has any further insight, please share, I'll get this hammered out some time this week.

geddski commented 12 years ago

The ideal is that the only difference in your test is that it's wrapped in a require() block. So if your test normally looks like this:

describe("regular test", function () {
    it("should work", function () {
        expect...
    });
});

And you want to test an AMD module, now your test looks like this:

require(['lib/mymodule'], function (module) {
  describe("regular test", function () {
    it("should work", function () {
        expect...
    });
  });
});

That's works with Jasmine and QUnit, and it's nice. I don't have to do anything different to test an AMD module, just wrap it in a require() block and "import" the JS file(s) I want to test. That's the ideal in my mind.

Currently the following actually works (using an externally hosted AMD module because of Issue #16)

describe("AMD", function () {
    it("should work", function (done) {
        require(['http://raw.github.com/gist/3eb65c2883af6b6f2b8d/41b6fbfaf24b8106268dd3c770ad22a976afcf3e/mymodule'], function (module) {
            expect(module.name).toEqual("A Module");
            done();
        });
    });
});

but it's backwards having the require() inside the define() and the it(). It also means I'd have to require the file a ton of times: inside of every single it().

This would be the ideal if we can make it work:

require(['http://raw.github.com/gist/3eb65c2883af6b6f2b8d/41b6fbfaf24b8106268dd3c770ad22a976afcf3e/mymodule'], function (module) {
    describe("AMD", function () {
        it("should work", function () {
            expect(module.name).toEqual("A Module");
        });
    });
});
cjohansen commented 12 years ago

Thanks for the input @geddesign. The "problem" with that solution when running through the server, is that Buster needs to have some idea what is supposed to happen and when. This is the background for my suggestion, in which Buster knows you have pending tests and wlil wait for them to complete.

We can build your suggested API as an AMD extension on top, but I don't want it hammered into Buster, cause we will need to wrap the require function to do that in a predictable way. By that I mean that you'll do (in your config file):

config["Browser tests"] = {
    environment: "browser",
    tests: ["**/*-test.js"],
    extensions: ["buster-amd"]
}
geddski commented 12 years ago

@cjohansen understood. Arriving at the ideal syntax with an extension sounds reasonable. And in the meantime your run() proposal looks pretty good, so long as you only need to do it once per file. So this:

describe("Something", function (run) {
    require(["something"], function (something) {
        run(function () {
            it("does things", function () {});
            it("does more things", function () {});
            describe('when hungry', function(){
                  it("does other things", function () {});
            });
        });
    });
});

and not this:

describe("Something", function (run) {
    require(["something"], function (something) {
        run(function () {
            it("does things", function () {});
        });
    });

    require(["something"], function (something) {
        run(function () {
            it("does more things", function () {});
        });
    });

     require(["something"], function (something) {
        run(function () {
            describe('when hungry', function(){
                  it("does other things", function () {});
            });
      });
});

You get the idea. A single run() would be totally fine.

cjohansen commented 12 years ago

Right, only a single run. Anyway, the extension will be small, you can expect it no later than a few days after the runner understands this stuff :)

geddski commented 12 years ago

Awesome. I want to get up to speed on the buster code base so I can contribute more meaningfully.

augustl commented 12 years ago

Awesome. I want to get up to speed on the buster code base so I can contribute more meaningfully.

Be careful, buster-capture-server and buster-resources is in dire need of refactoring ;)

geddski commented 12 years ago

Yep I'm cool with it, so long as I can have a single require and a single run per file as shown above.

geddski commented 12 years ago

Be careful, buster-capture-server and buster-resources is in dire need of refactoring

I ain't afraid :)

geddski commented 12 years ago

Running into any snags on this feature? It's the one thing preventing me from converting my projects over from Jasmine. Ok I'll stop buggin.

cjohansen commented 12 years ago

Got tangled up in paths in the config file, in addition to being slightly overloaded at work. I'm hoping to get this ironed out this week. Sorry for the delay!

johlrogge commented 12 years ago

Now that the relative path issue is solved I have managed to get a simple test to work. I have also played a bit with embracing the jsrequire model for loading dependenceis and while it is a bit involved I'm pretty pleased with what I have accomplished.

Here is my config:

config["web-module"] = {
    autoRun:false,
    environment: "browser",
    rootPath: "../web/resources/public/",
    tests: [
    "test/module/all-test.js"
    ],
    resources: ["**/*.js"],
    sources: ["js/require-jquery.js"]
}

As can be seen from the config I start everything from the all-test.js file in test/module. The all-test.js file looks like this:

require(["test/r-buster", "test/module/origin/storage-test"],
    function(buster) {
        // at this point we know all submodules has been loaded
        buster.run();
    });

Things to note:

  1. There are no tests in this file
  2. buster.run() is called in this module. This exploits the fact that js require guarantees that all child modules have been loaded before the parent module is initialized
  3. Consequently, the actual testcases are depended upon from the parent module. I have not verified this but it should work to also have child modules depend on other tests in their childmodules etc.
  4. buster is itself a dependency like any other requirejs dependency. This is not strictly necessary but is more "pure"

Now, lets look at how the buster module is defined

define([], function() {
    return buster;
});

We simply return the global buster object. This is a common trick in jsrequire but is not as good as having a module defining the buster object and returning it.

And now what you have all been waiting for. The testcase:

require(["test/r-buster", "js/origin/storage"],
    function(buster, storage) {
        buster.testCase("Some code module", {
        "will work": function () {
            assert(true);
        },
        "will work too": function () {
            refute(false);
        },
        "will fail": function() {
            assert.equals(storage, "this is a storage");
        }

        });

    });

Things to note:

Now finally the subject under test:

define(["jquery"],
       function($) {
       console.log("got here");
       return "this is a storage";
       });

I'm pretty happy about this. I think requirejs in itself gets us a long way and buster will know for sure that all tests are defined before run. We don't have to add an asyncTestCase or similar to user requirejs.

Now, the bad news:

requirejs defines the method "require" globally. This is no issue in browsertesting but it does mess up static-testing (and I would assume node-testing).

At the moment I can live with that but it would be a good thing to create a plugin from AMD... so how does one create plugins? Where do I start? :)

augustl commented 12 years ago

Why aren't you doing this?

buster.testCase("my test case", function (run) {
    require(["foo"], function () {
        run({
            // tests here ...
        });
    });
});
johlrogge commented 12 years ago
  1. Didn't know it was already implemented
  2. It is a bit backwards IMO, "my" way looks like any require JS-module and inside that module is what looks like any buster testcase. They don't overlap. The AMD-stuff is just wrapping the tests. I guess I share this opinion https://github.com/busterjs/buster/issues/15#issuecomment-3071165
  3. After working with requirejs for a while "my" way feels natural to me. It is what AMD modules look like

I reserve the right to have the opposite opinion tomorrow or in 5 minutes :)

augustl commented 12 years ago

Ah Chris only tweeted about it and hasn't updated this issue. I thought you were commenting because of the recent change that implemented this.

The idea is to patch require so that require("foo", function () {}) is basically the same as the example above. In other words just require, create buster.testCases in the callback, and nothing else is needed. No autoRun: false etc, just load the buster-requirejs plugin. This is just a small step two, now that async test cases are implemented this plugin should be easy to write. Baiscally it will do this:

(function () {
    var oldRequire = window.require;
    window.require = function (modules, cb) {
        buster.testCase.numTestCases++; // or something like that
        oldRequire(modules, cb);
    };
}());
johlrogge commented 12 years ago

That is pretty neat. Just realized that the requirejs-thingy automatically enables coffescript. Just install the cs-plugin and prepend cs! before the module name. Then the tests can be written like this:

define ['test/r-buster', 'js/origin/storage'],
       (buster, storage) -> 
        buster.testCase("some module", 
          {
            "will work": () -> assert(true),
            "will work too": () -> refute(false)
          })

freaky...

johlrogge commented 12 years ago

I tried the function(run) style and it did not work yet?

Uncaught exception: Uncaught Error: Tests should be an object

cjohansen commented 12 years ago

I sort of eagerly tweeted about the function (run) approach because I have it working on my machine :) But it's not complete, so I haven't pushed it yet.

Anyway, great work @johlrogge! I really like your approach, and agree that it's probably more the AMD way to do it that way. If you'd be interested in codifying it as a plugin that would be awesome. I can update this page: https://busterjs.org/docs/extensions/ to include what you need to know, I think we have all the API support you'd need to codify your approach.

I'm thinking: Configure buster like this:

config["web-module"] = {
    environment: "browser",
    extensions: ["buster-amd"],
    rootPath: "../web/resources/public/",
    libs: ["js/require-jquery.js"],
    tests: [
        "test/**/*.js"
    ],
    resources: ["**/*.js"]
}

The plugin will:

  1. Provide the buster module (like you specified it).
  2. Force autoRun to false.
  3. Generate the "all tests" meta package.

I like being specific in the config about what tests are to run. The plugin will use this information to build the meta package, but will remove them so they don't actually load. In code this means something along these lines:

var Path = require("path");

module.exports = {
    configure: function (config) {
        var testsToLoad, rootPath;

        config.on("load:tests", function (tests, rp) {
            rootPath = rp;
            testsToLoad = tests.slice();
            while (tests.length > 0) {
                tests.shift();
            }
        });

        config.on("load:resources", function (resourceSet) {
            testsToLoad.forEach(function (path) {
                resourceSet.addFile(Path.join(rootPath, path), {
                    path: path
                });
            });

            resourceSet.addResource("/load-all.js", {
                content: "Load all stuff here"
            });

            // Add buster module

            resourceSet.appendToLoad("/load-all.js");
        });
    }
};

I will complete my work on the async suites too, but I really dig this approach for the AMD tests.

johlrogge commented 12 years ago

I'm glad you like it. It is really encouraging! Of course I want to codify this as a plugin. I'll get cracking at it. I'll call the module buster-amd? (or is it requirejs specific?).

You will want to check my javascript code though :) I am a java/scala programmer mainly so I'm not too comfortable with javascript yet. I guess I'm saying, don't be shy with your comments about how my code can be improved. I know my skill is lacking and want to learn more.

cjohansen commented 12 years ago

Sure, we'll guide you through it :) I guess you can start it off as buster-requirejs, and we'll dub it buster-amd if it's possible to make one generic enough solution. I think so, but I'm not AMD-capable enough myself to say :)

geddski commented 12 years ago

@johlrogge great job, that was a clever idea! I replicated it locally as well. Just a couple of things we should consider:

Order:

The tests won't run in the same order every time. RequireJS loads dependencies async but guarantees no order (unless you use the order! plugin, which is kind of a hack). So if your all-test.js looks like this:

require(["lib/r-buster", "test/module-1", "test/module-2", "test/module-3"],
    function(buster) {
        buster.run();
});

sometimes module-1 will load first, sometimes module-3, etc. The order is as good as random. This shouldn't be a big deal for most people, especially since your tests are supposed to be independent of each other. But if you're using a certain reporter and want things to appear in the same order each time the tests are run you'd be out of luck. The reason RequireJS does this is so it can load the scripts in parallel, which is great for performance in the browser. If you want to guarantee the order of the tests you'd have to do this:

require(["require", "lib/r-buster"], function(require, buster) {
    require(["require", "test/module-1"], function(require){
        require(["require", "test/module-2"], function(require){
            // at this point we know all submodules has been loaded
            buster.run();
        });
    }); 
});

It's not super pretty, but it loads the scripts in order (and serially) just like regular script tags in an HTML page do. Downside is the tests may run slightly slower in the browser. But like I said, it's probably not an issue.

Config:

Like @cjohansen pointed out, there's something awesome about specifying what tests to run in the buster.js config file. If all-test.js could be generated from that config, that would be sweet indeed.

Great work! Let me know how I can help with the extension.

johlrogge commented 12 years ago

@geddesign Thanks!

I forgot to agree with Christian but his idea about dynamically generating the loader-module (all-tests) is just awesome. If it is dynamically generated it doesn't matter if it is ugly or verbose so we can use your strict ordering technique either as the norm or optional. I agree that one may want reporting to look pretty even if you have independent tests.

I have started a project and basically just created a module for it. That is I have all the boilerplate in place. I have also started to study buster-configuration since it seems that is what the plugin will make modifications to.

I guess I can just create a git repo o github that we can colaborate in until it is mature enough to make it into buster. Or, if I get access to some module repo either here or on gitorious would also work for me. Right now I have one locally on my machin so I just need to add a remote once one exists.

magnars commented 12 years ago

FYI Buster actually runs your tests in random order. Just so you don't rip your hair out in frustration testing the strict ordering technique above. :-)

geddski commented 12 years ago

@johlrogge cool. I'm also looking forward to trying out what @cjohansen has done with run(). His work may simplify the requirejs plugin quite a bit.

@magnars I thought that might be the case, good to know.

geddski commented 12 years ago

Fellas, I have another option for us: a simple requirejs plugin. Here's how you use it:

require(['test!mymodule'], function(module){
    //test the module, noob
});

The test! plugin loads your AMD module like normal but also lets buster know there's another test case to be aware of. The test! plugin is dead simple:

define({
    load:function (name, req) {
        /* TODO: increase the test case count
                    and anything else buster needs here
                    before requiring the module to be tested  */
        req([name], function (value) {
            load(value);
        });
    }
});

Here are the pros for this approach IMO:

The only missing magic is the code to tell buster about the new test case. @cjohansen, @augustl, what should that code look like?

johlrogge commented 12 years ago

That is an interesting idea! A couple of questions though (I'm probably just tired)

I thought about using a plugin. What I am not sure about though is: what if your test needs to load several dependenceies for one test? What we would need is to have the actual tests, not their dependencies loaded via the plugin right? That could be solved by using a loader module and depend on the tests via the test plugin but if we already have the loader module do we need the test-plugin?

Another approach would be to make a testplugin that loads several submodules. Basically checks the configuration and loads all the tests. That way we would not have to generate the loader module, it would always look the same. There may or may not be some benefit to this.

I may be overlooking something obvious here but it seems to me that the plugin approach would set a limitation on how many dependencies a test can have? ( at least if we want to the test count to correspond to the number of tests?).

I assume it is possible to chain plugins like this: test!cs!sut (where sut would be a coffeescript file). In this case ordering of plugins would maybe be important? Ie this: test!cs!sut would always work while cs!test!sut would sometimes start the testrun while the modules coffeescript is converted to javascript?

geddski commented 12 years ago

what if your test needs to load several dependenceies for one test?

I hadn't thought of that use case, thanks. I guess you could just use the test! plugin for the main module you are testing, so it only counts for one test:

require(['test!mymodule', 'logger'], function(module, logger){
    //test the module and log about it
});

Even if you're doing integration style tests and testing how three things work together, it's still just one test. So if you want the number of tests to be right use the plugin one time:

require(['test!part1', 'part2', 'part3'], function(part1, part2, part3){
    //test the integration of the parts
});

Hmmm...

geddski commented 12 years ago

Another downside the my plugin idea - you'd have to create a different plugin for loading/testing coffeescript.

Boy. The more I think about it, the more I'm happy with the run() solution for async test cases. Release it already @cjohansen! :)

cjohansen commented 12 years ago

My favorite contender so far is @johlrogge's plugin idea. But we're still pushing the run approach. I pushed the (almost) complete new runner yesterday: https://github.com/busterjs/buster-test/commit/3b3a71ff5ae35eb8e292f10ceb2af9ca27211e6b

I took the opportunity to make up for some pre-beta half assing, and rewrote the runner... Which is why this have taken so long ;)

johlrogge commented 12 years ago

@cjohansen I'm working on it and making progress. I have spent some time reading up on buster-configuration. I have not yet found where extensions are initialized but perhaps I wont have to know that...

The configuration and extension gets in

 configure: function (config)

is that an instance of the group in which the extension is specified? If I want to disacble autoRun do I just do:

config.autoRun = false;

I will probably have something after the hollidays unless I surprisemyself tonight

cjohansen commented 12 years ago

Cool. Yes, you get an instance of a group. autoRun is set on config.options.autoRun. autoRun is currently the only setting on the options object, but the idea is that this is where we'll put various user space configuration (like the upcoming timeout property and so on).

Extensions are loaded in the group: https://github.com/busterjs/buster-configuration/blob/master/lib/group.js#L171

johlrogge commented 12 years ago

Awesome, I think I know how I will implement it now. I'm not fast but I get the opportunity to learn some buster internals :)

cjohansen commented 12 years ago

That's awesome, it will help when you find more stuff you want to add later :)

cjohansen commented 12 years ago

I've pushed the run implementation: https://github.com/busterjs/buster-test/commit/3b3a71ff5ae35eb8e292f10ceb2af9ca27211e6b

However, I still need to make a few small changes before merging the branch and cutting a release.

johlrogge commented 12 years ago

I have something that appears to be working. It is hot from the oven but don't be shy to suggest improvements anyway :) I have just done happy path testing and it is likely that there are many ways to break this implementation but at least it appears to be working in the ONE situation I have tested it in :) I will try to use it in another project and see what I run into.

The code is pushed here https://github.com/johlrogge/buster-amd

My idea is that when you guys are happy with it you can fork from there and take ownership of the module.

I guess some documentation would not be a bad idea but this is how you use it:

var config = module.exports;
config["web-module"] = {
    environment: "browser",
    extensions: ["buster-amd"],
    rootPath: "../web/resources/public/",
    tests: ["test/module/**/*test.js"],
    resources: ["**/*.js"],
    libs: ["js/require-jquery.js"]
}

What it does:

What it should also do but doesn't do yet

It would be great if someone would like to try it out. Pull requests are most welcome if you find issues with it. It is not much code at all and I have written tests for the things that should be working ;o)

augustl commented 12 years ago

Great work! As discussed on IRC, the extension API will soon be changed a bit. But that shouldn't be hard to fix. In fact, it will hopefully make everything even more awesome.

cjohansen commented 12 years ago

This is great! Nice work, @johlrogge. The only thing I see lacking is automatically putting things in resources. You can keep a reference to the tests and then in load:resources add them back in using the resource set's addFile I guess.

I saw you discussing the functional approach in IRC. Maybe you're not aware, but you can safely use some higher-order array functions on node: https://gist.github.com/1523505

johlrogge commented 12 years ago

@cjohansen: Thanks! I did not know that array had these higher order functions in node. This is great news for me. I'm aware of the flaw with the resources, I'll attend to that tonight and push it together with the functionalification of the code.

It's been great fun to play with this and it will be great to be done with it soon :)

johlrogge commented 12 years ago

I pushed adding of the tests as resources. I also added a dependency decorator feature but I deferred the tests since it doesn't seem to be possible to add custom configuration for an extension.

cjohansen commented 12 years ago

@johlrogge Cool! I'm currently offline-ish while on vacation (and being sucked into Batman Arkham City, I have to admit :D ), I'll give it a spin when I'm back in business.

johlrogge commented 12 years ago

Hi, Hope you have enjoyed your vacation and offlineishness. Have you tried buster-amd? I have used it some and it sort of works. Today I ran into situations when i would like to replace some modules with stubbed-modules but I think that would require being able to configure those since it s too late at load-time...

Just a friendly ping to see that buster-amd is not forgotten :)

augustl commented 12 years ago

We're still working on the rewrites of buster-capture-server and buster-resources, the goal is to be finished with these no later than feb 18th. When we are, we'll start looking into making the buster-amd module work 100%, document it, provide stable APIs, etc.

We'll certainly let you know when we do, and we really appreciate the effort!

johlrogge commented 12 years ago

Sweet, Tell me if there is something I can do. I guess I could document how it is used at the very least. Anything else?

cjohansen commented 12 years ago

Joakim: certainly not forgotten. My plan was to send you a pull request when the new apis are ready. I think everything except a few details on the server are ready now, so expect it soon.

geddski commented 12 years ago

Glad this is still on the radar. @augustl The capture server is being flaky for me: it randomly stops working and I have to restart the capture server/reconnect my browsers about every 20 minutes. I'll wait till the rewrite lands though to report issues.