Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AfterAll hook #1011

Closed
wants to merge 3 commits into from
Closed

AfterAll hook #1011

wants to merge 3 commits into from

Conversation

wjlroe
Copy link

@wjlroe wjlroe commented Aug 22, 2016

Summary

Carrying on the work done in #858. This pull request aims to add a hook to replace using Kernel#at_exit for the end of a cucumber suite's cleanup tasks.

Motivation and Context

The problem with using Kernel#at_exit for test suite cleanup tasks is that it is very easy to accidentally clobber the exit status of the test suite. If you call any code that throws (and catches) exceptions, that will effectively overwrite the $ERROR_INFO value and turn an error status into a success status, which you likely don't want.

Due to the tricky way in which developers have to treat Kernel#at_exit, it would be preferable for cucumber to provide a hook for the same use cases except the exit status of the test suite would be preserved - anything that happens in the hook, you can't "break" the expected behaviour of cucumber.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've added acceptance tests for my code
  • I've added specs for my code
  • I've completed the implementation for this feature
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • The name of the hook has been discussed and agreed upon.

@wjlroe
Copy link
Author

wjlroe commented Aug 22, 2016

My next action is to dive into the specs to drive these tests closer to the implementation, it's not going to be super quick because I'm working on this in fits and bursts here and there.

@mattwynne
Copy link
Member

👍 @wjlroe

Feel free to ping me here on in gitter if you need some help finding your way around. There are some fairly gnarly bits of code you'll be getting into.

@mattwynne mattwynne mentioned this pull request Sep 2, 2016
@mattwynne
Copy link
Member

How are you getting on @wjlroe? Need any help?

The AfterAll hook does fire, but presumably the output isn't captured
properly since the features don't pass (you can see the puts output
above the actual feature output)
@wjlroe
Copy link
Author

wjlroe commented Sep 2, 2016

@mattwynne I've just had a try at implementing this hook. It doesn't pass the tests, but you can see it is kinda working. I don't really understand what's going on though, so some pointers would be appreciated. The output from the AfterAll hooks doesn't get included in the output captured when the feature tests are run (but the puts code does execute), I don't know why that is. When I have a better understanding of all this, I'll add more specs, but right now this is exploratory code.

@@ -67,6 +67,7 @@ def run!
receiver = Test::Runner.new(@configuration.event_bus)
compile features, receiver, filters
@configuration.notify :test_run_finished
fire_after_all_hook
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved up one line, so that the test_run_finished event means "all userland code that will be executed has been executed".

@brasmusson
Copy link
Contributor

brasmusson commented Sep 4, 2016

I have argued elsewhere that it should be possible by listening to the event stream exactly what userland code is executed (which would include AfterAll hooks). That means that we need new xxStarted and xxFinished events for these hooks that are not TestSteps (which Before, After and AfterStep hooks are), that are fired similar to the TestStepStarted and TestStepFinished events for TestSteps

When an AfterAll hook fails (an exception escapes it) I expect that the exit code in non-zero and the exceptions is captured by the formatters (by listening to the new xxFinished event).

@mattwynne
Copy link
Member

Good point Bjorn. I wonder if we could abstract the name 'test step' so that it could cover steps in scenarios, before / after scenario hooks, and these new after all hooks?

@andrii-rymar
Copy link

@wjlroe do you have any plans to continue working on this?

@wjlroe
Copy link
Author

wjlroe commented Feb 3, 2017

I don't know what to do, the discussion above doesn't mean anything to me, and I'm not likely to get any time to work on this, so no.

@mattwynne
Copy link
Member

Thanks for your efforts so far @wjlroe and sorry we lost you in the feedback discussion. I'm sure we can take it from here.

@gewargise
Copy link

Just wondering if there were any updates regarding an AfterAll hook?

@alexttransisland
Copy link

alexttransisland commented Sep 18, 2017

is it still valid? I was able to use

AfterConfiguration do |config|
  config.on_event :test_run_finished do |event|
    config.out_stream.puts "test run finished"
  end
end

with 3.0.0.pre.2 version.
Isn't it the same?

@stale
Copy link

stale bot commented Nov 17, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 17, 2017
@stale
Copy link

stale bot commented Nov 24, 2017

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

@lock
Copy link

lock bot commented Nov 24, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⌛ stale Will soon be closed by stalebot unless there is activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants