Check exit status in ruby_exe

[andrykonchin]

Raise RuntimeError when ruby_exe run Ruby script and it exits with failed status.

Close ruby/spec#767

Example

bin/mspec ../spec/library/net/ftp/default_passive_spec.rb $ ruby /Users/andrykonchin/projects/mspec/bin/mspec-run ../spec/library/net/ftp/default_passive_spec.rb ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin18] 1) Net::FTP#default_passive is true by default ERROR RuntimeError: ruby_exe(/Users/andrykonchin/.rbenv/versions/2.7.1/bin/ruby /Users/andrykonchin/projects/spec/library/net/ftp/fixtures/default_passive.rb) failed: #<Process::Status: pid 75873 exit 4> /Users/andrykonchin/projects/spec/library/net/ftp/default_passive_spec.rb:6:in `block (2 levels) in <top (required)>' /Users/andrykonchin/projects/spec/library/net/ftp/default_passive_spec.rb:4:in `<top (required)>' [| | ==================100%================== | 00:00:00] 0F 1E  Finished in 0.176300 seconds  1 file, 1 example, 0 expectations, 0 failures, 1 error, 0 tagged 

[eregon]

The error message should show the command that failed

[eregon]

FWIW

$ ruby -e 'system "exit 3", exception: true' Traceback (most recent call last):	1: from -e:1:in `<main>' -e:1:in `system': Command failed with exit 3: exit 3 (RuntimeError) 

But I find it rather unclear.

So I'm thinking something like:
"ruby_exe(#{command.inspect}) failed: {$?.inspect}"

[andrykonchin]

Done

[eregon]

We should make sure all cases in ruby/spec are fixed before merging this to not break the CI there.
Could you start a PR to ruby/spec addressing them?

(maybe we should do a run of ruby/spec here to make sure mspec changes don't break ruby/spec)

[eregon]

(maybe we should do a run of ruby/spec here to make sure mspec changes don't break ruby/spec)

#51

[eregon]

I was hesitating for the name of the keyword argument (e.g. continue_on_failure), but since there is system(cmd, exception: true) now in Ruby 2.7+ that seems the obvious choice 👍 (except we'll default it to true)

[andrykonchin]

Could you start a PR to ruby/spec addressing them?

Will work on it soon.

[eregon]

I would show the created command here, because it's easier to read and we need it anyway.

command = ruby_cmd(code, opts) output = `#{command}` if !$?.success? && exception raise "ruby_exe(#{command.inspect}) failed: #{$?.inspect}"

[eregon]

That has the advantage that if there is some issue behavior in ruby_cmd it should be obvious from the output, and make it easier to run the command on its own.

[andrykonchin]

Done

[eregon]

I wonder if the option should be something like ruby_exe(..., exit_code: 1) and expect exit_code: 0 as default.
Either way, ruby_exe would check the $?.exitstatus against that.

That way, we would always test the exit code, rather than exception: false some cases, which ignores the exit status, unless it's also tested after explicitly.
@andrykonchin What do you think about that?

[eregon]

Another advantage compared to exception: false usages, is if the subprocess would unexpectedly exit with exit code 0, it would fail the spec.

One issue with that approach is it might not be easy to give an exact exit code, e.g. for ruby -e "raise SignalException, 'SIGKILL'".
Actually, in such a case $?.exitstatus is nil:

ruby -e 'system "ruby", "-e", "raise SignalException, %{SIGKILL}"; p $?; p $?.exitstatus' #<Process::Status: pid 17803 SIGKILL (signal 9)> nil 

So we could accept exit_code: nil for the cases of dying from a signal.

[andrykonchin]

Yeah, I see. It definitely makes sense to check result exit code (if it's the same on different platforms).

But I would also consider separating 1) helper (ruby_exe) and 2) expectations. It may be more flexible to add new matchers but it adds some complexity.

I mean something like this:

result = ruby_exe(command, options) result.should have_execution_output('') result.should be_successful_execution result.should be_failed_execution result.should have_execution_exit_status(0) result.should have_execution_signal(9)

[eregon]

Yeah, I see. It definitely makes sense to check result exit code (if it's the same on different platforms).

Yes they should be the same, because either the program is successful (then it's 0), an exception reached the top level (then it's 1), or there is some explicit exit N, or it died from a signal and then the exit code is nil.

But I would also consider separating 1) helper (ruby_exe) and 2) expectations. It may be more flexible to add new matchers but it adds some complexity.

Yes, that's an important concern.
I was thinking for instance we should probably not "count" the expectation about the exit status in ruby_exe, so something like:

it "foo" do ruby_exe("bar") end

still says "no expectations in example".

It's true that keeping ruby_exe is kind of nice, but OTOH it seems a good thing to ensure the exit code is what is expected, which motivated me to file ruby/spec#767 (probably I noticed some specs actually returned exit code 1 but did not intend it).

Also, something related, in case the command fails, it's very useful if we can show the output of stderr, instead of having to search that through the log. This is potentially something ruby_exe() could do if it checks the exit code (for example by $stderr.reopen(StringIO) around calling the subprocess). So if the exit code doesn't match, we'd show the exit code does not match message + the stderr output. I'd need to play around with that for the specifics.

So, I think ruby_exe(..., exit_code: 0) is preferable to ruby_exe(..., exception: true), could you try that?
I'm sorry to change my mind quite late about this.

[eregon]

Regarding naming, I'm happy with either ruby_exe(..., exit_code: 0) or ruby_exe(..., exit_status: 0).
(ruby_exe(..., exitstatus: 0) seems not so readable)

[andrykonchin]

I see. Will try.

[eregon]

@andrykonchin Could you try the exit_code/exit_status approach? I'd be happy to merge your work here :)

[andrykonchin]

Yes, sure. I will work on it. Hope this week. Sorry for delaying.

[andrykonchin]

@eregon I added the exit_status option and updated ruby/spec#819. Could you please take a look?

So now we can safely merge ruby/spec#819 and then ensure this PR passes the CI checks on the new RubySpec master branch.

The only issue I noticed is that exit status is checked explicitly in some tests:

 before :each do ruby_exe("exit(42)", exit_status: 42) end it "returns the process exit code" do $?.exitstatus.should == 42 end

I noticed such checks in the following files:

• core/kernel/at_exit_spec.rb
• core/process/status/exitstatus_spec.rb
• core/process/status/exited_spec.rb
• core/process/status/to_i_spec.rb
• core/process/status/success_spec.rb
• shared/process/exit.rb
• core/exception/system_exit_spec.rb

[eregon]

@andrykonchin Looks great, thank you!

[eregon]

(I amended and pushed the last commit to rerun CI)