Mock not working for Process.run with block.

[postmodern]

I am attempting to stub Process.run, but the variant which receives a block. I cannot find an example of how to successfully define the stub method which accepts the block and how to expect/allow the stub method to receive a block? My ultimate goal is to test when Process.run raises a File::NotFoundError exception and whether my code correctly rescues that exception and raises another more descriptive exception.

require "./spec_helper"

Spectator.describe Test do
  mock Process do
    stub self.run(command,shell,output,&block)
  end

  let(command) { "ls -l" }

  before_each do
    expect(Process).to receive(:run).with(command, shell: true, output: :pipe)
  end

  it "must stub Process.run" do
    Process.run(command, shell: true, output: :pipe) do |process|
    end
  end
end

Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro 'stub'

Code in spec/test_spec.cr:6:11

 6 | end
             ^
Called macro defined in lib/spectator/src/spectator/mocks/stubs.cr:3:13

 3 | private macro stub(definition, *types, _file = __FILE__, _line = __LINE__, return_type = :undefined, &block)

Which expanded to:

 > 26 | __temp_72.mocks.record_call(self, __temp_74)
 > 27 | if (__temp_75 = __temp_72.mocks.find_stub(self, __temp_74))
 > 28 |   return __temp_75.call!(__temp_73) { previous_def { |*__temp_76| yield *__temp_76 } }
                                              ^-----------
Error: there is no previous definition of 'run'

[postmodern]

I get the same error when trying to use receive(:run) { ... }.

    expect(Process).to(receive(:run) { |command,shell,output|
    })

Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro 'stub'

Code in spec/test_spec.cr:6:11

 6 | end
             ^
Called macro defined in lib/spectator/src/spectator/mocks/stubs.cr:3:13

 3 | private macro stub(definition, *types, _file = __FILE__, _line = __LINE__, return_type = :undefined, &block)

Which expanded to:

 > 26 | __temp_72.mocks.record_call(self, __temp_74)
 > 27 | if (__temp_75 = __temp_72.mocks.find_stub(self, __temp_74))
 > 28 |   return __temp_75.call!(__temp_73) { previous_def { |*__temp_76| yield *__temp_76 } }
                                              ^-----------
Error: there is no previous definition of 'run'

[icy-arctic-fox]

Stubs with blocks are partially implemented and honestly a bit broken. There's some work done on master that attempts to resolve this. Issue #42 details similar problems with block stubs. There's a fix in the works.

[icy-arctic-fox]

Providing an update here since it's been a while.

I have decided enough is enough and I've invested effort into redoing the entire mock and stub system. Instead of trying to fix this issue with the current system, it will be fixed in the new system. Progress on the new system is going well, but I don't have an estimate on when it will be ready.

[icy-arctic-fox]

Providing another update here as it has been a while. Life events have been going on, so I'm mostly busy with that, but it's wrapping up.

The main functionality for mocks and doubles has been re-implemented. It takes advantage of new Crystal macro features that have been added since the initial implementation of mocks and doubles.

Doubles will work mostly the same as they have before. They'll be a lot more robust.

Mocks have had the most changes. When defining a mock for an existing type, all methods have stub functionality applied automatically. It now looks something like this:

context "mocks" do
  class MyClass
    def foo
      "foo"
    end
  end

  mock(MyClass) # or def_mock(MyClass)
  let(fake) { mock(MyClass) } # or new_mock(MyClass)

  it "can stub methods" do
    expect { allow(fake).to receive(:foo).and_return("bar") }.to change { fake.foo }.from("foo").to("bar")
  end
end

Default stubs can be provided as well.

mock(MyClass, foo: "bar")

And for more complex defaults, there's a new stub syntax:

mock(MyClass) do
  stub def foo
    "bar" # Do complex stuff here.
  end
end

The PR containing these changes is here. I am trying to address all previous mock, double, and stub issues raised with the new system.

The remaining work mostly consists of re-implementing matchers and spies. This work should be simple.

What won't be in the initial overhaul is mocking modules and concrete structs. There is some functionality for "injecting" mockability into concrete types, but it needs refinement.

[icy-arctic-fox]

The overhauled mock and double system is merged to master. There are some things to cleanup and document, then it will be ready for a release. Until the official docs are available, the DSL specs can be used as a reference.

[icy-arctic-fox]

I've looked into this issue with the new mocks, and there's some issue I'm not seeing a cause for.

Theoretically this code should work:

Spectator.describe Test do
  inject_mock Process do
    # Instance variable that can be nil, provide a default.
    @process_info = Crystal::System::Process.new(0)
  end

  let(command) { "ls -l" }
  let(exception) { File::NotFoundError.new("File not found", file: "test.file") }

  before_each do
    expect(Process).to receive(:run).with(command, shell: true, output: :pipe).and_raise(exception)
  end

  it "must stub Process.run" do
    Process.run(command, shell: true, output: :pipe) do |process|
    end
  end
end

Through some debugging, the stub for Process.run isn't being applied, even though the method definition matches exactly. I don't understand what's going on here, but I plan to revisit this later.

[postmodern]

Guessing it's because there are two Process.run methods, one that accepts a block and one that doesn't but instead returns a Process::Status value?

[icy-arctic-fox]

Both of those are getting stubbed. In case you (or anyone else) is interested, here's what I've found.

Dumping the class methods of Process with:

macro finished
  {% puts Process.resolve.class.methods %}
end

produces:

# omitting irrelevant methods...
def self.run(command : String, args = nil, env : Env = nil, clear_env : Bool = false, shell : Bool = false, input : Stdio = Redirect::Close, output : Stdio = Redirect::Close, error : Stdio = Redirect::Close, chdir : Path | String | ::Nil = nil) : Process::Status
  status = (new(command, args, env, clear_env, shell, input, output, error, chdir)).wait
  $? = status
  status
end

def self.run(command : String, args = nil, env : Env = nil, clear_env : Bool = false, shell : Bool = false, input : Stdio = Redirect::Pipe, output : Stdio = Redirect::Pipe, error : Stdio = Redirect::Pipe, chdir : Path | String | ::Nil = nil)
  process = new(command, args, env, clear_env, shell, input, output, error, chdir)
  begin
    value = yield process
    $? = process.wait
    value
  rescue ex
    process.terminate
    raise(ex)
  end
end

def self.run(command : String, args = nil, env : Env = nil, clear_env : Bool = false, shell : Bool = false, input : Stdio = Redirect::Close, output : Stdio = Redirect::Close, error : Stdio = Redirect::Close, chdir : Path | String | ::Nil = nil) : Process::Status
  __temp_511 = ::Spectator::Arguments.capture(command, args, env, clear_env, shell, input, output, error, chdir)
  __temp_512 = ::Spectator::MethodCall.new(:run, __temp_511)
  _spectator_record_call(__temp_512)
  if __temp_513 = _spectator_find_stub(__temp_512)
    _spectator_cast_stub_value(__temp_513, __temp_512, typeof(previous_def(command, args, env, clear_env, shell, input, output, error, chdir)), :raise)
  else
    _spectator_stub_fallback(__temp_512, typeof(previous_def(command, args, env, clear_env, shell, input, output, error, chdir))) do
      previous_def(command, args, env, clear_env, shell, input, output, error, chdir)
    end
  end
end

def self.run(command : String, args = nil, env : Env = nil, clear_env : Bool = false, shell : Bool = false, input : Stdio = Redirect::Pipe, output : Stdio = Redirect::Pipe, error : Stdio = Redirect::Pipe, chdir : Path | String | ::Nil = nil)
  __temp_514 = ::Spectator::Arguments.capture(command, args, env, clear_env, shell, input, output, error, chdir)
  __temp_515 = ::Spectator::MethodCall.new(:run, __temp_514)
  _spectator_record_call(__temp_515)
  if __temp_516 = _spectator_find_stub(__temp_515)
    _spectator_cast_stub_value(__temp_516, __temp_515, typeof(previous_def(command, args, env, clear_env, shell, input, output, error, chdir) do |*_spectator_yargs|
      yield *_spectator_yargs
    end), :raise)
  else
    _spectator_stub_fallback(__temp_515, typeof(previous_def(command, args, env, clear_env, shell, input, output, error, chdir) do |*_spectator_yargs|
      yield *_spectator_yargs
    end)) do
      previous_def(command, args, env, clear_env, shell, input, output, error, chdir) do |*_spectator_yargs|
        yield *_spectator_yargs
      end
    end
  end
# ...

There are four methods - the two originals and the two Spectator defined to wrap the originals with mock functionality. The method signature matches between the original and override pairs. What's also interesting is that most other methods in the list do not have their original implementation. Only the new methods that Spectator defined are present. For instance, Process.fork does not have the original implementations listed.

def self.fork : Process
  __temp_505 = ::Spectator::Arguments.capture
  __temp_506 = ::Spectator::MethodCall.new(:fork, __temp_505)
  _spectator_record_call(__temp_506)
  if __temp_507 = _spectator_find_stub(__temp_506)
    _spectator_cast_stub_value(__temp_507, __temp_506, typeof(previous_def() do |*_spectator_yargs|
      yield *_spectator_yargs
    end), :raise)
  else
    _spectator_stub_fallback(__temp_506, typeof(previous_def() do |*_spectator_yargs|
      yield *_spectator_yargs
    end)) do
      previous_def() do |*_spectator_yargs|
        yield *_spectator_yargs
      end
    end
  end
end

def self.fork : Process | ::Nil
  __temp_508 = ::Spectator::Arguments.capture
  __temp_509 = ::Spectator::MethodCall.new(:fork, __temp_508)
  _spectator_record_call(__temp_509)
  if __temp_510 = _spectator_find_stub(__temp_509)
    _spectator_cast_stub_value(__temp_510, __temp_509, typeof(previous_def()), :nil)
  else
    _spectator_stub_fallback(__temp_509, typeof(previous_def())) do
      previous_def()
    end
  end
end

I believe it is due to this that the mock isn't working. The original method is being called instead of the override that Spectator defined. This is despite it having the exact method signature as the original. Maybe I'm missing something obvious from staring at this code so long.

[icy-arctic-fox]

I believe I've found the issue. When a nilable type is expanded with a macro, it changes to a union of nil (makes sense). So String? becomes String | Nil essentially when the macro is expanded. The Process.run method's last argument is:

chdir : Path | String? = nil

which gets expanded to:

chdir : Path | String | ::Nil = nil

We can see this in the snippet from my previous comment.

The problem is that to some part(s) of the compiler, the two are not equivalent. Here's some debug output produced by a macro that iterated over the original method and corresponding method that Spectator produced:

Original == Generated ?
"command : String" == "command : String" : true
"args = nil" == "args = nil" : true
"env : Env = nil" == "env : Env = nil" : true
"clear_env : Bool = false" == "clear_env : Bool = false" : true
"shell : Bool = false" == "shell : Bool = false" : true
"input : Stdio = Redirect::Pipe" == "input : Stdio = Redirect::Pipe" : true
"output : Stdio = Redirect::Pipe" == "output : Stdio = Redirect::Pipe" : true
"error : Stdio = Redirect::Pipe" == "error : Stdio = Redirect::Pipe" : true
"chdir : Path | String | ::Nil = nil" == "chdir : Path | ::Nil = nil" : false

Surprisingly, even though the expanded code for chdir on both is exactly the same, the arguments are not considered equal. This is because the original still uses Path | String? internally, but Spectator produced Path | String | Nil. This is why there appears to be two pairs of identical methods. They're actually different, but get displayed with the expanded | Nil syntax.

As a sanity check, I changed my copy of process.cr to use Path | String | Nil and the example compiled and behaved as expected. Instead of being two pairs of methods, there are only two methods - the Spectator generated replacements.

I could technically work around this issue in Spectator. But I feel this should be fixed in the compiler instead. I'll do some more digging to see if this could be fixed easily in the compiler.

Technically the arguments look like this to the compiler: Original: Union(Path, Union(String, Nil)) Generated: Union(Path, String, Nil)

[icy-arctic-fox]

This has been mostly fixed as-of Crystal 1.6.0 (https://github.com/crystal-lang/crystal/issues/12330). Keyword arguments can't be used in place of positional arguments. This is something I plan to add soon (#47). In the meantime, the following works:

inject_mock Process do
  # Instance variable that can be nil, provide a default.
  @process_info = Crystal::System::Process.new(0)
end

let(command) { "ls -l" }
let(exception) { File::NotFoundError.new("File not found", file: "test.file") }

before_each do
  pipe = Process::Redirect::Pipe
  expect(Process).to receive(:run).with(command, nil, nil, false, true, pipe, pipe, pipe, nil).and_raise(exception)
end

it "must stub Process.run" do
  expect do
    Process.run(command, shell: true, output: :pipe) do |_process|
    end
  end.to raise_error(File::NotFoundError, "File not found")
end

Closing since the main issue is fixed, but continuing work in #47.