Handle failures in BeforeAll hooks and AfterAll hooks#1857
Conversation
* All BeforeAll hooks and AfterAll hooks shall be executed regardless of they pass or fail. * The TestCases shall only be executed if all BeforeAll hooks passes. * BeforeAll hooks shall be executed after the TestRunStarted event has fired. * AfterAll hooks shall be executed before the TestRunFinished event is fired. * Print the global hook failures shall be printed by the console formatters. * Aslo global hook failures shall affect the exit code.
luke-hill
left a comment
luke-hill
left a comment
There was a problem hiding this comment.
Once v11 is out I'll get to this. Might take me a week or two
luke-hill
left a comment
luke-hill
left a comment
There was a problem hiding this comment.
Few minor questions. Great stuff though
|
|
||
| def on_test_case_started(event) | ||
| @current_test_case_started_id = test_case_started_id(event.test_case) | ||
| @current_test_run_hook_started_id = nil |
There was a problem hiding this comment.
This shouldn't be needed
There was a problem hiding this comment.
In the attach method we need to know if the attachment comes from an before_all/after_all hook or from a test case step of hook. Since we store both current_test_case_started_id and current_test_run_hook_started_id can be set when test step or hooks are executed and when after all hooks are executed. In this pull request the solution is to use the condition if @current_test_run_hook_started_id.nil? to decide whether it is a test step or hook, or a before all hook or after all hook that is executing. Using that condition (in the attach method) requires that we ensure that @current_test_run_hook_started_id is nil when a test step or hook is executed. That is the purpose of this line.
| file_name: filename, | ||
| timestamp: time_to_timestamp(Time.now) | ||
| } | ||
| attachment_data = if @current_test_run_hook_started_id.nil? |
There was a problem hiding this comment.
To avoid crazy indents can we do something like this
| attachment_data = if @current_test_run_hook_started_id.nil? | |
| attachment_data = | |
| if @current_test_run_hook_started_id.nil? | |
| { | |
| ..... | |
| } |
|
|
||
| def before_all | ||
| set_up_world_for_global_hooks | ||
| all_succeded = true |
There was a problem hiding this comment.
Typo *succeeded (In other spots too)
| rescue StandardError => e | ||
| @configuration.notify(:test_run_hook_finished, hook, Core::Test::Result::Failed.new(timer.duration, e)) | ||
| raise | ||
| false |
There was a problem hiding this comment.
So this is where the magic happens/happened. Thanks for this spot!
| end | ||
|
|
||
| def before_all | ||
| set_up_world_for_global_hooks |
There was a problem hiding this comment.
Can you explain the goal here of this bit (Both the setup and the clearer on line 156 for this method)
There was a problem hiding this comment.
Calling attach from hooks and step definition works since the world object handles the attach and creates an event/message for it. To be able to call attach from before all and after all hooks, I create a world object here so something exist to create the event/message for the attach calls in before all hooks.
|
|
||
| load_step_definitions | ||
| fire_install_plugin_hook | ||
| fire_before_all_hook unless dry_run? |
There was a problem hiding this comment.
Is this meant to be deleted?
There was a problem hiding this comment.
Yes. The ndjson files of the cck dev kit specifies that the expected message sequence is that the TestRunHookStarted/Finished messages for the before all hook come after the TestRunStarted message. Therefore I have moved the execution of the before all hooks to after the TestRunStarted event has been sent. The TestRunStarted event is broadcast by one of the filters (BroadcastTestRunStartedEvent) close to the end of the filter chain, so I have therefore created the FilreBeforeAllHooks filter which is placed directly after the BroadcastTestRunStartedEvent filter. Therefore before all hooks shall not be fired from here.
* Fix spelling of succeeded * Change indentation
2 participants
Description
With this change Cucumber-Ruby conforms fully with CCK v22
Type of change
Please delete options that are not relevant.
Checklist:
Your PR is ready for review once the following checklist is
complete. You can also add some checks if you want to.
bundle exec rubocopreports no offenses