Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Before and After hooks. #31

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

tristandunn commented Oct 20, 2011

Before and After hooks would help clean up some code in my pig project, so I'm attempting to add them. I have most of it working and tested, but it got a little tricky towards the end. I'd appreciate some feedback on whether I am heading in the right direction or any suggestions on how to improve the implementation.

@jbpros jbpros referenced this pull request Oct 20, 2011

Closed

Hooks support #32

Owner

jbpros commented Oct 20, 2011

Hi Tristan,

I only had a quick look at what you did but it looks pretty good so far.

As a general suggestion, I think you should stick with the domain term hook instead of callback.

A callback is any function that should be called by another function. In Cucumber's vocabulary, a hook is a specific callback being fired when certain events occur during feature execution.

Does it make sense?

I'll give you more fine-grained feedback tonight.

Thanks for the nice work!

@jbpros jbpros and 1 other commented on an outdated diff Oct 20, 2011

lib/cucumber/support_code/hook.js
@@ -0,0 +1,18 @@
+var Hook = function(type, code) {
+ var Cucumber = require('../../cucumber');
+
+ var self = {
+ invoke: function(world, callback) {
+ try {
+ code.apply(world, [callback]);
+ } catch(exception) {
@jbpros

jbpros Oct 20, 2011

Owner

I'm not sure how exceptions should be handled here, if they should at all. I discussed it a few minutes with @msassak and we thought exceptions within hooks should not be handled. But after some trials on Cucumber 1.1, it seems they actually are: they halt the current scenario but not the whole Cucumber thread.

What's certain is that there is no such behaviour in the hooks.feature file. I.e. this part of the code is not tested :) I suggest to remove this catch block for now, until we know exactly what to do.

@tristandunn

tristandunn Oct 20, 2011

Contributor

Removing it seems reasonable since it isn't currently defined.

Owner

jbpros commented Oct 20, 2011

I think there is a conceptual mistake in the current implementation. Before and After hooks are supposed to be fired before and after scenarios. If I get it right, the hooks are currently fired before and after features.

Features are actually nonexistent when it comes to hooks. You can only hook around steps, scenarios and the whole suite run.

Let's get back to this project of yours for a minute. Do you actually need suite, feature or scenario hooks there?

Contributor

tristandunn commented Oct 20, 2011

Wow, not sure how I missed that. I'll get it changed to scenario soon.

Ideally I need before and after scenario with tags, but I can get away without tags or even at the suite level for now.

@jbpros jbpros and 1 other commented on an outdated diff Oct 21, 2011

lib/cucumber/runtime/ast_tree_walker.js
@@ -46,7 +46,21 @@ var AstTreeWalker = function(features, supportCodeLibrary, listeners) {
var event = AstTreeWalker.Event(AstTreeWalker.SCENARIO_EVENT_NAME, payload);
self.broadcastEventAroundUserFunction(
event,
- function(callback) { scenario.acceptVisitor(self, callback); },
+ function(callback) {
+ supportCodeLibrary.getBeforeHooks().forEach(function(beforeHook, callback) {
+ beforeHook.invoke(world, callback)
+ }, function() {
+ scenario.acceptVisitor(self, function() {
+ supportCodeLibrary.getAfterHooks().forEach(function(afterHook, callback) {
+ afterHook.invoke(world, callback)
+ }, function() {
+ if (callback) {
+ callback();
@jbpros

jbpros Oct 21, 2011

Owner

Nested functions quickly become hard to read, IMO. I've used the extract method refactoring pattern in several spots (e.g. just like in the AstTreeWalker). Basically you move down the first nested callback to a wrapSomething() function and repeat that extraction until the end of the callback chain.

It's not perfect but I think it reduces the noise and complexity of such code fragments.

WDYT?

@tristandunn

tristandunn Oct 21, 2011

Contributor

Definitely agree. I knew this part was going to need some clean up. I was already thinking about switching to

supportCodeLibrary.triggerBeforeHooks(world, function() {
  scenario.acceptVisitor(self, function() {
    supportCodeLibrary.triggerAfterHooks(world, callback);
  });
});

Combining that with extracting the callbacks it'll be a lot cleaner and substantially easier to test.

Owner

jbpros commented Oct 22, 2011

Nice work! Thanks Tristan.

The next step would be to go green against the related hooks.feature's scenarios.

You've added JS step definitions to features/step_definitions/cucumber_steps.js, which is a good start.

However, hooks.feature is part of the cucumber-features suite. Implementations are expected to pass against the cucumber-features suite run by Cucumber-ruby. Running it through the implementation itself is a secondary objective (but definitely great and important).

In short, the step definitions are to be written in Ruby, in
features/cucumber-features/step_definitions/cucumber_stepdefs.rb. So yeah, not in cucumber.js's own repository, but in cucumber-features so that they are reused by all implementations.

The step definitions there generally call a single method that is called a mapping. A few of them are common to all implementations (see cucumber_mappings.rb) while the others are specific to implementations (in cucumber.js, you'll find them in features/step_definitions/cucumber_js_mappings.rb).

As you'd have to send pull requests on cucumber-features, I suggest you create a temporary cucumber_js_hooks_mappings.rb file or something in cucumber-js's repo. We'll move the step definitions to cucumber-features's cucumber_mappings.rb when we're ready to merge your work.

I hope this is not too confusing. Feel free to ping me if you need help!

Contributor

tristandunn commented Oct 27, 2011

I assume I need to extract some of the step definitions to the hook mappings, as suggested, but am I on the right track?

Owner

jbpros commented Oct 28, 2011

You're on the right track indeed. Excellent work so far!

I've refactored the step definitions a bit to be more consistent with the rest of them. I suggest you merge these changes into the pull request. Have a look at it and feel free to ask questions if you have any.

Contributor

tristandunn commented Dec 5, 2011

This has fallen pretty far behind master. Am I missing anything before I attempt to rebase my branch?

Owner

jbpros commented Dec 5, 2011

There were no major changes since your last commit. I think you should be able to merge/rebase quite easily.

Contributor

tristandunn commented Dec 5, 2011

That was easier than expected. Let me know if you see anything funky.

dbrock commented Jan 8, 2012

Any further progress on this?

Owner

jbpros commented Jan 8, 2012

@dbrock I'm now reviewing the latest commits, it's a highly requested feature.

Contributor

tristandunn commented Jan 14, 2012

I've been rather quiet lately, but let me know if there's anything else I can help out with.

@jbpros jbpros commented on the diff Jan 15, 2012

lib/cucumber/runtime/ast_tree_walker.js
@@ -46,7 +46,17 @@ var AstTreeWalker = function(features, supportCodeLibrary, listeners) {
var event = AstTreeWalker.Event(AstTreeWalker.SCENARIO_EVENT_NAME, payload);
self.broadcastEventAroundUserFunction(
event,
- function(callback) { scenario.acceptVisitor(self, callback); },
+ function(callback) {
+ supportCodeLibrary.triggerBeforeHooks(world, function() {
+ scenario.acceptVisitor(self, function() {
+ supportCodeLibrary.triggerAfterHooks(world, function() {
+ if (callback) {
@jbpros

jbpros Jan 15, 2012

Owner

Any reason for this condition?

@tristandunn

tristandunn Jan 17, 2012

Contributor

I want to say I originally added this to get a test passing, but not 100% sure. If that's the case then the test case should be improved and this should be removed.

@jbpros

jbpros Jan 17, 2012

Owner

I'm in the middle of a merge from master onto your branch and I removed this condition. It seems to work well.

+1. very much looking forward to this addition to wire in database cleaner after tests.

@jbpros jbpros closed this in 2c3b5e2 Jan 18, 2012

Owner

jbpros commented Jan 18, 2012

@tristandunn Thank you for your nice work on this.

Contributor

tristandunn commented Jan 19, 2012

Awesome! Glad to help.

great work guys - fantastic to see a much requested feature arrive so quickly.

Owner

jbpros commented Jan 19, 2012

Well it actually took a long time to get merged, mostly because of a crazy end of year on my side ;)

We still need to implement the other types of hooks though (see #32).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment