Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Add a spec for interrupt trapping in Runner #2096

Closed
wants to merge 47 commits into from

Conversation

myrridin
Copy link
Contributor

The Runner.trap_interrupt method was missing a spec. My goal is to extend interrupt handling per tenderlove's feature request rspec/rspec#13, but I wanted to make sure there was a spec in place first.

@myrridin
Copy link
Contributor Author

It looks like this failed some Ruby versions in the Travis CI build, namely all JRuby and 1.9.2 MRI. I'm going to investigate to see if I can determine the issue.

@JonRowe
Copy link
Member

JonRowe commented Oct 22, 2015

The 1.9.2 build is possibly just a flake, and it's entirely possible that JRuby doesn't support signal handling

@myrridin
Copy link
Contributor Author

JRuby claims to, though I wouldn't be surprised if there were differences. I'm currently working to diagnose the JRuby issue.

If it turns out that this is non suppported, what's your recommendation? Is the spec not viable, or is it possible to limit this to MRI? What about 1.9.2? I'm having trouble getting the script to even run in that version though it's obviously setup on my end causing the issue.

@myrridin
Copy link
Contributor Author

I can confirm that I'm able to trap SIGINT in a basic Ruby script with jruby 1.7.19 (1.9.3p551), and changing the trap syntax to pass in a proc object doesn't seem to help at all. Unfortunately I'm probably beyond my capacity on this one, but will continue poking.

@myrridin
Copy link
Contributor Author

I have this gist which runs successfully. Per Charles Nutter I'm filing this as a JRuby bug, but don't seem to be able to reproduce in just JRuby. jruby/jruby#3413

@fables-tales
Copy link
Member

@myrridin is this still something you'd like to continue with? Would love to see this merged

@myrridin
Copy link
Contributor Author

I can definitely take a fresh look. I've got an issue open on the jruby repo, but this doesn't appear to be an actual jruby bug. I'll do what I can to get this across the finish line. Thanks for checking in :-)

@fables-tales
Copy link
Member

@myrridin cool, thanks. Are you likely to be able to push more code in the next week?

@myrridin
Copy link
Contributor Author

If I'm going to get it, it will be in the next week. If my second attempt doesn't work out I'll have to pass along as beyond my capabilities (or temporal capacity) :-) I'll do my best to update here regardles of the outcome.

@myrridin
Copy link
Contributor Author

I changed up my approach here. This is borrowed directly from Rubocop: https://github.com/bbatsov/rubocop/blob/33aa8bcc3f50e545f97043b7eb7fd7228c4476da/spec/rubocop/cli_spec.rb#L2176

The idea is that instead of sending a SIGINT directly to the process as it runs, you capture the interrupt handlers that were created and manually call them.

The other solution was to add a sleep into the previous test, which felt wrong to do. The problem with signal handling is that there's no way I could figure out to block until the signal was processed. If I artificially delayed before the end of the spec I had full green every time, but the whole approach seemed to be going about it in the wrong way.

A friend of mine pointed me to this file and it seemed like a much better way to go about it.

@myrridin
Copy link
Contributor Author

That passes jruby but fails MRI now :-(

Please hold. I will definitely complete this. On a mission now.

myrridin and others added 18 commits December 10, 2015 22:20
The Runner.trap_interrupt method was missing a spec. My goal is to extend
interrupt handling per tenderlove's issue #2055, but I wanted to make sure
there was a spec in place first.
This avoids a problem some users have seen where
files are double-required.

Fixes rspec#2099.
... by using Array for lines until final rendering.
It allows detail or failure/error line to be used as the first line
by omitting preceding lines, rather than specifying content of detail or
failure/error as description.
jackscotti and others added 23 commits December 10, 2015 22:22
Ripper might fail to parse a Ruby source even if the current runtime
parsed it properly, because some versions of JRuby have different
implementation of runtime parser and Ripper parser.

jruby/jruby#2427

So we handle the syntax error in Ripper and fall back to the simple
single line extraction in that case.
- Better warning when an invalid value is passed
  for `--fail-fast` (including silencing call site
  since it will not be in user code).
- No need to stub `reporter.failures_required`.
- No need for the `failures_required` method at all.
- Use `fail_fast_limit_met?` in more situations.
- Remove unneeded `ExampleGroup.fail_fast?` method.
The return value of RubyProject.add_to_load_path is neither useful nor
used anywhere. In this case, #each communicates intent better than #map.
The existing wording was confusing for this situation:

* You have two specs, a and b.
* Spec a always fails.
* Spec b fails only if a runs first.

In such a situation, your failures are order dependent,
but our bisector printed "failure is not order dependent",
which was quite confusing. The new wording,
"failures(s) do not require any non-failures to run first"
is more accurate.
`Exception.new.backtrace` is `nil` so we can't count on
there always being a backtrace. Before now we relied
on the `format_backtrace` caller handling this (passing
`ex.backtrace || []`), but with rspec#2074 now also including
the `exception.cause` in failure output, the code there
did not guard the `format_backtrace` call with `|| []`
and is causing users to get a `NoMethodError`. It's
easiest if `format_backtrace just handles the `nil` case
so each caller does not have to remember to handle it.

Fixes rspec#2117.
This is useful because at the moment the only way to determine if an
example is currently executing a context hook is to look at it's inspect
string. Here, we add a simple boolean flag which is triggered in before
and after context hook execution.

This is needed by rspec/rspec-rails#1501.
@myrridin
Copy link
Contributor Author

I went back to the basic version and tried to rebase it, along with a merge from master. I'm not used to working on larger repos like this, so let me know if I did something I shouldn't have. Expect these to pass this time, though I'll keep an eye on them.

Edit: Oh jeez, that destroyed the PR view :-( Should I just extract the specs into a new branch and resubmit a new PR?

@myrridin
Copy link
Contributor Author

Ok I remade this PR to clean up the git issues I introduced: #2132

@myrridin myrridin closed this Dec 11, 2015
@JonRowe
Copy link
Member

JonRowe commented Dec 11, 2015

Thanks :)

@myrridin myrridin deleted the interrupt-handling branch December 11, 2015 13:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants