Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bypass running specs as if they were pending with Ctrl + T #741

Closed
wants to merge 1 commit into from

6 participants

@matthewrobertson

This pull request implements a simple feature idea I would love to see in rspec:

Sometimes I have individual specs that hang or take forever. This commit allows users to bypass a long running specs by hitting Ctrl + t and sending the INFO interrupt signal. Currently I am just marking them as pending but down the road you might consider adding more comprehensive reporting. Let me know what you think.

@myronmarston

I think this is a great idea and I like seeing how simple the implementation is :).

However, it completely broke the travis build....looks like SIGINFO isn't available on some systems? You should probably use one of the standard POSIX signals. Not sure which one is most appropriate...maybe USR1?

Anyhow, it'd be great to also have some kind of test coverage for this. Maybe add a cucumber feature that defines a long running spec (using sleep), sends the appropriate signal, and then asserts that the spec was pending as a result?

@alindeman
Collaborator

It's worth noting that minitest uses SIGINFO to print information about the current test that's running (and current errors and failures too).

We don't necessarily have to follow their precedent, of course.

It's nice that SIGINFO has a key combination (on OS X anyway). I think USR1 would require a separate kill -USR1, right?

@myronmarston

A key combo is really nice. But the code as currently exists raises an error when the signal handler is registered. So it won't work on all systems (such as ubuntu, apparently--that's what the Travis VMs run, I think).

Maybe it should be a config option? That way on OS X users could configure SIGINFO so they have a signal with a key combo; on another system, they can choose an appropriate signal. Making it a config option would also surface it so people are aware of it, particularly if we put it in the generated spec_helper.rb file.

Also, I notice that the first-pass code by @matthewrobertson sets the signal handler before each example. I don't think we need to set it more than once, do we? So setting it once when configured would be preferable, I think.

@matthewrobertson

Seems like I broke some builds here :trollface:. I will investigate making it configurable.

@matthewrobertson

@myronmarston can you have a look at this now and let me know what you think? Here are the changes:

  • I am only trapping the interrupt while a spec is running, after each run I reset it to 'DEFAULT'
  • I am only trapping interrupts if they are defined on the system
  • If SIGINFO is not available, I fall back to SIGSTP which can be triggered via Ctrl+z on most systems

If you are happy with this I could add some specs. If you want to make it configurable, I will need you to point me in the direction of where that should happen.

lib/rspec/core/example.rb
((5 lines not shown))
+ if trap_type
+ trap(trap_type[:signal]) { raise ExampleInterrupted.new(trap_type[:message]) }
+ end
+ end
+
+ def unset_interrupt_trap
+ trap(trap_type[:signal], 'DEFAULT') if trap_type
+ end
+
+ def trap_type
+ if(Signal.list['INFO'])
+ { :signal => 'INFO', :message => 'Skipped at runtime via Ctrl+T'}
+ elsif(Signal.list['TSTP'])
+ { :signal => 'TSTP', :message => 'Skipped at runtime via Ctrl+Z'}
+ end
+ end
@myronmarston Owner

Given that this method is called twice for each example, it concerns me that it re-checks the condition every single time, when the list of signals supported by a particular host computer is never going to change between examples.

I'd prefer to see the signal list checked once, at startup time, and the signal/message values stored, then re-used for each example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rspec/core/example.rb
@@ -309,6 +311,24 @@ def run_after_each
@example_group_instance.teardown_mocks_for_rspec
end
+ def set_interrupt_trap
+ if trap_type
+ trap(trap_type[:signal]) { raise ExampleInterrupted.new(trap_type[:message]) }
+ end
+ end
+
+ def unset_interrupt_trap
+ trap(trap_type[:signal], 'DEFAULT') if trap_type
+ end
+
+ def trap_type
+ if(Signal.list['INFO'])
+ { :signal => 'INFO', :message => 'Skipped at runtime via Ctrl+T'}
+ elsif(Signal.list['TSTP'])
+ { :signal => 'TSTP', :message => 'Skipped at runtime via Ctrl+Z'}
@myronmarston Owner

Ctrl-Z already has a well defined behavior of suspending the foreground process. Overriding it here could be potentially confusing for end users. Thoughts?

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

The more I think about it, I don't feel great about using SIGINFO or SIGTSTP for this purpose. Other tools use seem to use SIGINFO to dump some information about what they're doing, not to affect execution in any way. And SIGTSTP is normally used to temporarily suspend execution.

The convenience of a keystroke is not yet outweighing the bad feeling I get when I think of violating common Unixisms.

Thoughts?

@myronmarston

This is definitely an improvement! I left a few comments.

@alindeman -- I'm on the fence about that, but I am concerned about overiding Ctrl-Z, which usually suspends the process, to make it do something different.

That fact that we're both on the fence about which signals to use suggests that maybe a config option is the way to go? Or do you think it's better just to pick a signal for users?

@matthewrobertson

@alindeman just a note: the SIGINFO interrupt still dumps the same info to the console whether we trap or not, ie:

load: XXX cmd: ruby XXX running XXXu xxxs

UPDATE

Actually that is not true, the behaviour before we trapped was no output....

@matthewrobertson

@myronmarston now I feel like Travis-CI is just messing with me. Any hints as to why the build is still broken?

@myronmarston
Owner

@matthewrobertson -- the build looks like it's failing due to a weird rbx compilation error. I think it's intermittent.

@alindeman - any more thoughts about what signal to use and/or whether or not we should make it configurable?

@alindeman
Collaborator

If configurable, what would be the defaults?

@JonRowe
Owner

OMG I want this... If it could silence some of the output too that'd be even more awesome! (Currently I have test suites where outputting backtraces to the console takes magnitudes of time longer than actually running the test suite...)

@samphippen
Collaborator

My feeling here is that siginfo and sigstp probably aren't meant to be used for this purpose. As cool as this is, it makes my neckbeard twitch with worry. If there was a "sigfinishasquicklyasyoucanplz" that'd be the right signal to send. I definitely think this shouldn't be the default, but would be ok with a config option.

@samphippen
Collaborator

Regarding what the defaults are:

default: no crazy ctrl-thing key handling
configuration option: something like catches_siginfo

@JonRowe
Owner

Ctrl-\ is QUIT on OSX... would seem appropriate?

@myronmarston
Owner

I'd still like to get this (or something like it) merged. The issues we need to address are:

  • Lack of tests. We don't ever merge new features w/o corresponding tests. Some specs would be nice, and I'd really like some cukes that demonstrate this end-to-end. I think aruba (the library we use for our cukes) has signal support so we should be able to demonstrate it end to end with that.
  • Make it configurable. I'm not sure what the config API should be yet, but as commented on #520, there's a desire to make the existing ctrl-c signal handler configurable, too.
  • Decide if this is the behavior we actually want for this extra signal. Minitest does something different: it dumps info about the current running example but doesn't cause it to be skipped. I'm on the fence about what behavior we want here.
  • Docs are needed as well.

This issue isn't a priority for me (lots of other things to work on) but I hope we can get this integrated at some point!

@JonRowe
Owner

I don't think we should use Ctrl-T, as that is for more information, we should map a more appropriate signal like QUIT

@xaviershay
Collaborator

Closing, stale.

@xaviershay xaviershay closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 3, 2012
  1. @matthewrobertson
This page is out of date. Refresh to see the latest.
Showing with 31 additions and 1 deletion.
  1. +14 −0 lib/rspec/core/configuration.rb
  2. +17 −1 lib/rspec/core/example.rb
View
14 lib/rspec/core/configuration.rb
@@ -900,6 +900,20 @@ def order_groups_and_examples(&block)
order_examples(&block)
end
+ # @private
+ def skip_example_trap
+ if @skip_example_trap.nil?
+ if(Signal.list['INFO'])
+ @skip_example_trap = { :signal => 'INFO', :message => 'Skipped at runtime via Ctrl+T'}
+ elsif(Signal.list['TSTP'])
+ @skip_example_trap = { :signal => 'TSTP', :message => 'Skipped at runtime via Ctrl+Z'}
+ else
+ @skip_example_trap = false
+ end
+ end
+ @skip_example_trap
+ end
+
private
def get_files_to_run(paths)
View
18 lib/rspec/core/example.rb
@@ -112,7 +112,7 @@ def run(example_group_instance, reporter)
begin
run_before_each
@example_group_instance.instance_eval(&@example_block)
- rescue Pending::PendingDeclaredInExample => e
+ rescue ExampleInterrupted, Pending::PendingDeclaredInExample => e
@pending_declared_in_example = e.message
rescue Exception => e
set_exception(e)
@@ -296,11 +296,13 @@ def record_finished(status, results={})
end
def run_before_each
+ set_interrupt_trap
@example_group_instance.setup_mocks_for_rspec
@example_group_class.run_before_each_hooks(self)
end
def run_after_each
+ unset_interrupt_trap
@example_group_class.run_after_each_hooks(self)
verify_mocks
rescue Exception => e
@@ -309,6 +311,18 @@ def run_after_each
@example_group_instance.teardown_mocks_for_rspec
end
+ def set_interrupt_trap
+ if trap_type = RSpec.configuration.skip_example_trap
+ trap(trap_type[:signal]) { raise ExampleInterrupted.new(trap_type[:message]) }
+ end
+ end
+
+ def unset_interrupt_trap
+ if trap_type = RSpec.configuration.skip_example_trap
+ trap(trap_type[:signal], 'DEFAULT')
+ end
+ end
+
def verify_mocks
@example_group_instance.verify_mocks_for_rspec
rescue Exception => e
@@ -327,5 +341,7 @@ def record(results={})
execution_result.update(results)
end
end
+
+ class ExampleInterrupted < StandardError ; end
end
end
Something went wrong with that request. Please try again.