SIGINT trap consistent with TERM behaviour

[akostadinov]

Summary

fixes #988

Details

Basically now first Ctrl+C will terminate scenario execution but will stil execute at_exit hooks. Second Ctrl+C will also interrupt that.

Motivation and Context

see #988

In the past first Ctrl+C only told Cucumber to exit after this scenario. And a second one terminated immediately without at_exit hooks. So there was no way to interrupt immediately but still execute at_exit. This could lead to tested system to remain in inconsistent state and require manual restore.

How Has This Been Tested?

Not sure how can I test this.

I editted my local copy of the gem and run a test scenario

$  cucumber features/long_feature.feature 
Feature: Long running scenarios

  @clean-up
  Scenario: 100 seconds with clean-up # features/long_feature.feature:4
    Given I love you                  # features/step_definitions/gah.rb:1
      hey
^C
Exiting... Interrupt again to exit immediately.
    When I wait 100 seconds           # features/step_definitions/gah.rb:8
      exit (SystemExit)
      ./features/step_definitions/gah.rb:9:in `sleep'
      ./features/step_definitions/gah.rb:9:in `/^I wait (\d+) seconds$/'
      features/long_feature.feature:6:in `When I wait 100 seconds'
    Then I do nothing                 # features/step_definitions/gah.rb:5
     --->  I cleaned you <---

Failing Scenarios:
cucumber features/long_feature.feature:4 # Scenario: 100 seconds with clean-up

1 scenario (1 failed)
3 steps (1 failed, 1 skipped, 1 passed)
0m2.806s

Screenshots (if appropriate):

Types of changes

• Refactor (code change that does not change external functionality)
• 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)

Not sure about the above.

Checklist:

• I've added tests for my code
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[luke-hill]

I'll pull this down and give it a quick play.

EDIT: Could you maybe write a changelog entry for this, as it's definitely new behaviour

[luke-hill]

So I couldn't get the new behaviour (Adding your one line didn't affect things for me), Possibly due to me not often using at_exit hooks.

# sample.feature

Feature: Test
  Scenario: One
    Given I say hello

  Scenario: Two
    Given I say hello

# sample_steps.rb
Given('I say hello') do
  puts 'hello'
end

Before do
  puts 'start of scenario'
end

After do
  puts 'I am sleeping after the scenario'
  sleep 3
end

at_exit do
  puts 'I am a slow at_exit hook'
  sleep 3
end

Running without your code yielded the following
1 (Plain run)

start of scenario
hello
.
I am sleeping after the scenario
start of scenario
hello
.
I am sleeping after the scenario

2 scenarios (2 passed)
2 steps (2 passed)
0m6.014s
I am a slow at_exit hook

2 (Ctrl+C)

start of scenario
hello
.
I am sleeping after the scenario
^C
Exiting... Interrupt again to exit immediately.

1 scenario (1 passed)
1 step (1 passed)
0m3.004s
I am a slow at_exit hook

This suggests to me that without your code, Ctrl+C does still run the global teardown hooks.

If I've got something wrong, please let me know. I've also tried with your code and didn't notice any difference.

[akostadinov]

Forgot to write I have used this project as test:
https://github.com/akostadinov/testcuke

First of all the intention of the change is when hitting Ctrl+C during scenario execution, not during After hook. But as I see from your log the behaviour still makes sense. You have interrupted what is currently going on and go directly to at_exit for clean-up.

In my project I notice that After hook is executed at_exit so if you do Ctrl+C during scenario, then After hook will still be executed. If I hit Ctrl+C during After hook, then interrupting it and going to at_exit makes sense to me.

In the past one hits Ctrl+C but nothing apparently happens. You still need to wait for things to happen as normal. Second hit will stop everything wihtout a chance for clean-up. That's what I would like to see fixed. And this will be as far as I can tell consistent with kill my_cucumber_process.

I think if you use my project with wait times of 100 seconds, it will be more apparent. Please let me know what you think, I can add release notes. But I would appreciate to tell me what text will be desired more than SIGINT trap consistent with TERM behaviour which is already the commit message.

Thank you for looking into this!

[luke-hill]

Hi There,

Yes you are right, my example was bad. I've tried a new one with a slow scenario (Not before/after), and it works exactly as you described.

Can you just add something into the Changelog, explaining the before/after. Pretty much as you just have here. I'll get this merged in once that's done.

Cc/ @tooky I've tested this locally and it works exactly as he has described. This is a good bugfix. (Beforehand it was actually broken)

[akostadinov]

Done, let me know if wording is good.

[olleolleolle]

Tiny formatting detail: if you remove the newline at line 28, the bullet list becomes 1 bullet list, making formatting nicer.

[luke-hill]

Just git commit amend this @akostadinov and I'll approve and merge this in.

[akostadinov]

Didn't notice that! fixed now

[aslakhellesoy]

Hi @akostadinov,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

[luke-hill]

Merged. Thanks for your contribution

[lock]

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.