Add plugins support #127

Saulis · 2015-03-24T13:26:20Z

Hi!

Needed to do some plugins for gemini (starting up express, sauce-connect, etc) so I made a really simple plugin loader. What do you think?

review-ninja · 2015-03-24T13:26:20Z

Saulis · 2015-03-24T13:32:59Z

Will fix the style errors asap

coveralls · 2015-03-24T14:20:22Z

Coverage increased (+0.43%) to 62.14% when pulling 718f1ca on Saulis:feature/plugins into e26f42e on bem:master.

SevInf · 2015-03-24T18:58:32Z

Thanks, this is a great addition. Will review now

SevInf · 2015-03-24T19:04:06Z

test/plugins.test.js

+            var options = {plugins: {foobar: true}};
+            plugins.load(this.gemini, options);
+
+            assert(this.foobarPlugin.calledWith(this.gemini, {}));


Can you please use sinon.assert and js-must in tests? The rest of the tests are using this assertions.

SevInf · 2015-03-24T19:43:06Z

Overall I like the idea of ability to launch webserver, sauceconnect or build tool automatically, but I don't think startRunner event handler is appropriate place to do so. Note that if we are doing long-running async tasks in EventEmmiter's handler, .emit won't wait unitl it ends and just continue execution. So, for example, tests might start before sauceconnect is finished launching.

I see two options here:

Look for some EventEmmiter subclass that allows to return a promise from event event handler and waits for it to be resolved. There are few such libraries on npm, one of it might help us.
Don't use events at all, just allow to set beforeAll and afterAll callbacks that will be executed in appropriate moments.

What do you think?

SevInf · 2015-03-24T19:45:31Z

lib/gemini.js

-    var _this = this;
+module.exports = inherit(EventEmitter, {
+    __constructor: function(config, overrides) {
+      config = config || DEFAULT_CFG_NAME;


Function body should be indented by 4 spaces. Not sure why jshint didn't notifiy you about it, will check the settings

Yes I share your concerns - a problem indeed arises with sauce-connect because it needs to finish before running the tests should begin. I thought about it, but originally couldn't find a suitable place in gemini to hook a callback or a promise to it.
So, instead I was planning just to block the execution in the sauce plugin until it's ready - and yes, I'd really rather do it properly :-)

I think I'll start by looking for options on how to do it using promises.

At some point I was considering adding calls before and after calling the gemini.test but dropped the idea afterwards because then it wouldn't work when running tests programmatically using the API, right?

Found something called chained-emitter which seems to do the trick:
var ChainedEmitter = require('chained-emitter').EventEmitter
then, we can do this:

return _this.emit('startRunner', runnerInstance) .then(function() { return _this.readTests(paths); }) .then(function(rootSuite) { options.reporters.forEach(applyReporter.bind(null, runnerInstance)); return runnerInstance.run(rootSuite); }) .then(function(data) { _this.emit('endRunner', runnerInstance, data); return data; });

What still bothers me though a bit is the order of the promises.. I would like to emit the 'startRunner' after _this.readTests(paths) but then we have to rely that the plugins event handler passes through the rootSuite to the next step and making that assumption just doesn't feel right. The same issue is with the 'endRunner' emitter, the 'data' would need to passed through it. Any thoughts on this?

From the plugins perspective, it looks like this currently:

gemini.on('startRunner', function(runner) { var deferred = Q.defer(); server.start(opts, function(rootUrl) { gemini.config.rootUrl = rootUrl; deferred.resolve(); }); return deferred.promise; });

Here's few ways I achieved the running order I was looking for. They're both ugly, do you know if there is a way to do the same thing with some built-in method found in Q/Promise ?

Nested:

return _this.readTests(paths) .then(function(rootSuite) { return _this.emit('startRunner', runnerInstance) .then(function () { options.reporters.forEach(applyReporter.bind(null, runnerInstance)); return runnerInstance.run(rootSuite); }); }) .then(function(data) { return _this.emit('endRunner', runnerInstance, data) .then(function() { return data; }); });

Using temp vars:

return _this.readTests(paths) .then(function(rootSuite) { _this.rootSuite = rootSuite; }) .then(function() { return _this.emit('startRunner', runnerInstance); }) .then(function() { options.reporters.forEach(applyReporter.bind(null, runnerInstance)); return runnerInstance.run(_this.rootSuite); }) .then(function(data) { _this.data = data; return _this.emit('endRunner', runnerInstance, data); }) .then(function() { return _this.data; });

I think your first example can be rewritten as:

return _this.readTests(paths) .then(function(rootSuite) { return _this.emit('startRunner', runnerInstance).thenResolve(rootSuite); }) .then(function (rootSuite) { options.reporters.forEach(applyReporter.bind(null, runnerInstance)); return runnerInstance.run(rootSuite); }) .then(function(data) { return _this.emit('endRunner', runnerInstance, data).thenResolve(data) });

which looks fine to me.

At some point I was considering adding calls before and after calling the gemini.test but dropped the idea afterwards because then it wouldn't work when running tests programmatically using the API, right?

Yes, you are totally right, if we are adding plugins they should be loaded when using gemini via API, CLI and GUI

Oh yes, thenResolve() was exactly what I needed, thanks!

Update: chained-emitter depends on bluebird which apparently doesn't have a thenResolve method.

I guess we'll just have to do a

.then(function() { return rootSuite; });

I'll see if I have time to send a pull request to bluebird at some point.

- Use sinon.assert - Use _.startsWith in resolving packageName. - Refactor pairs usage into a chain.

…sting#127)

coveralls · 2015-03-25T20:47:49Z

Coverage increased (+0.34%) to 62.05% when pulling 600b34b on Saulis:feature/plugins into e26f42e on bem:master.

coveralls · 2015-03-27T14:13:13Z

Coverage increased (+1.54%) to 63.25% when pulling 310072a on Saulis:feature/plugins into 7e8f99b on bem:master.

SevInf · 2015-03-30T11:44:09Z

@Saulis can you please send suiteName-related commits as a separate PR?

SevInf · 2015-03-30T11:49:02Z

test/plugins.test.js

+        it('should throw error if plugin not found', function() {
+            var options = {plugins: {'gemini-foo': true}};
+
+            try {


it can be written as:

(function() { plugins.load(this.gemini, options); }).must.throw()

SevInf · 2015-03-30T11:53:35Z

Overall, it is ready to be merged, but I really want to finish with 0.10.0-final before adding new features. This will take 1-2 days and then I'll merge this PR and publish it as 0.10.1. Are you ok with it?

jouni · 2015-03-30T12:20:14Z

@Saulis is away this week, so I’ll provide a comment on his behalf, that 1–2 days delay is totally okay :)

Saulis · 2015-03-30T13:00:54Z

I might have some time tomorrow to polish things up :)

@SevInf yeah I'll make a separate PR, and I'll remove the related changes from this one with a force push

…sting#127)

coveralls · 2015-03-30T18:48:04Z

Coverage increased (+0.34%) to 62.05% when pulling 4fe4f52 on Saulis:feature/plugins into 7e8f99b on bem:master.

SevInf · 2015-04-06T17:31:10Z

Sorry for delay, I'm finally done with 0.10. I'm ready to merge this PR, but it needs rebase. Will you do it?
I can rebase it too, but if you have some other commits we will end up with diverged history.

Saulis · 2015-04-06T18:33:48Z

Sure, just a sec

- add startRunner and endRunner events. - add chained-emitter to support promises in event handlers.

Saulis · 2015-04-06T19:50:28Z

I had to squash the commits into one to make the merge process easier

coveralls · 2015-04-06T20:00:30Z

Coverage increased (+0.33%) to 63.02% when pulling 6329f19 on Saulis:feature/plugins into 00a0300 on bem:master.

Saulis · 2015-04-07T04:40:23Z

Hmm, I might've screwed something up with the merge, need to check it one more time.

Update: nevermind it's good, ready to merge :-)

Add plugins support

SevInf · 2015-04-07T12:19:33Z

Thanks!

SevInf mentioned this pull request Mar 24, 2015

Assertions style #128

Closed

SevInf reviewed Mar 24, 2015
View reviewed changes

SevInf changed the title ~~Feature/plugins~~ Add plugins support Mar 25, 2015

Saulis added a commit to Saulis/gemini that referenced this pull request Mar 25, 2015

- Fix minor issues (gemini-testing#127)

52b6aa2

- Use sinon.assert - Use _.startsWith in resolving packageName. - Refactor pairs usage into a chain.

Saulis added a commit to Saulis/gemini that referenced this pull request Mar 25, 2015

Add chained-emitter to support promises in event handlers. (gemini-te…

600b34b

…sting#127)

Saulis force-pushed the feature/plugins branch 2 times, most recently from d56c6be to 310072a Compare March 27, 2015 14:11

SevInf reviewed Mar 30, 2015
View reviewed changes

Saulis force-pushed the feature/plugins branch from 310072a to fa36da8 Compare March 30, 2015 18:40

Saulis added a commit to Saulis/gemini that referenced this pull request Mar 30, 2015

Add chained-emitter to support promises in event handlers. (gemini-te…

fa36da8

…sting#127)

Saulis force-pushed the feature/plugins branch from fa36da8 to 4fe4f52 Compare March 30, 2015 18:46

Saulis added a commit to Saulis/gemini that referenced this pull request Mar 30, 2015

Add chained-emitter to support promises in event handlers. (gemini-te…

4fe4f52

…sting#127)

Saulis mentioned this pull request Mar 30, 2015

Add suiteFullName property to test related events. #131

Closed

Add plugins support. (gemini-testing#127)

6329f19

- add startRunner and endRunner events. - add chained-emitter to support promises in event handlers.

Saulis force-pushed the feature/plugins branch from 4fe4f52 to 6329f19 Compare April 6, 2015 19:49

SevInf pushed a commit that referenced this pull request Apr 7, 2015

Merge pull request #127 from Saulis/feature/plugins

6c3cf8b

Add plugins support

SevInf merged commit 6c3cf8b into gemini-testing:master Apr 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugins support #127

Add plugins support #127

Saulis commented Mar 24, 2015

review-ninja commented Mar 24, 2015

Saulis commented Mar 24, 2015

coveralls commented Mar 24, 2015

SevInf commented Mar 24, 2015

SevInf Mar 24, 2015

Saulis Mar 25, 2015

SevInf commented Mar 24, 2015

SevInf Mar 24, 2015

Saulis Mar 25, 2015

Saulis Mar 25, 2015

Saulis Mar 25, 2015

SevInf Mar 25, 2015

SevInf Mar 25, 2015

Saulis Mar 25, 2015

coveralls commented Mar 25, 2015

coveralls commented Mar 27, 2015

SevInf commented Mar 30, 2015

SevInf Mar 30, 2015

SevInf commented Mar 30, 2015

jouni commented Mar 30, 2015

Saulis commented Mar 30, 2015

coveralls commented Mar 30, 2015

SevInf commented Apr 6, 2015

Saulis commented Apr 6, 2015

Saulis commented Apr 6, 2015

coveralls commented Apr 6, 2015

Saulis commented Apr 7, 2015

SevInf commented Apr 7, 2015

Add plugins support #127

Add plugins support #127

Conversation

Saulis commented Mar 24, 2015

review-ninja commented Mar 24, 2015

Saulis commented Mar 24, 2015

coveralls commented Mar 24, 2015

SevInf commented Mar 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SevInf commented Mar 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 25, 2015

coveralls commented Mar 27, 2015

SevInf commented Mar 30, 2015

Choose a reason for hiding this comment

SevInf commented Mar 30, 2015

jouni commented Mar 30, 2015

Saulis commented Mar 30, 2015

coveralls commented Mar 30, 2015

SevInf commented Apr 6, 2015

Saulis commented Apr 6, 2015

Saulis commented Apr 6, 2015

coveralls commented Apr 6, 2015

Saulis commented Apr 7, 2015

SevInf commented Apr 7, 2015