define() method has a bug

[XSven]

I would like to mock the stat() CORE subroutine in the Bar module

use strict;
use warnings;
package Bar;
sub size_of_file { ( stat $_[ 0 ] )[ 7 ] }
1;

This is my failing test script

use strict;
use warnings;

use Test::More tests => 3;

use Test::MockModule qw();

my $class;
my $mock;

BEGIN {
  $class = 'Bar';
  my $code = sub ( ;* ) { CORE::stat $_[ 0 ] };
  $mock = Test::MockModule->new( $class, no_auto => 1 )->define( stat => $code );
  #$mock->{ _orig }{ stat } = $code;
  use_ok $class;
}

my $expected_size;
$mock->redefine(
  stat => sub ( ;* ) {
    return ( ( undef ) x 7, $expected_size );
  }
);
my $method = 'size_of_file';
$expected_size = 42;
is $class->can( $method )->( __FILE__ ), $expected_size, 'check size (mock)';

$mock->unmock( 'stat' );
is $class->can( $method )->( __FILE__ ), -s __FILE__, 'check size (unmock)';

Uncommenting the line

#$mock->{ _orig }{ stat } = $code;

makes the test script pass.

What is the strategy of my test script: In the BEGIN block (compile time) before loading the Bar module I have used define() to add a wrapper subroutine (it wraps CORE::stat) with the name stat to the Bar module. Then I have loaded the Bar module. At run time I have used redefine() to mock the wrapper subroutine. I have done an is-test. Then I have unmock()ed the wrapper subroutine. My expectation was that unmocking brings back the implementation of the wrapper subroutine. Unfortunately define() does

test-mockmodule/lib/Test/MockModule.pm

Line 144 in a2d8108

$self->{_orig}{$name} = undef;

instead of

test-mockmodule/lib/Test/MockModule.pm

Line 142 in a2d8108

$self->{_orig}{$name} = \&$sub_name;

To me this is a bug caused by the fact that the implementation of define() is based on _mock().

[atoomic]

At first glance this looks a bug in your code here.

You try to mock the CORE::stat function but your test here is not mocking the CORE::stat function but the main::stat function. This is why when you setup a previous value for it then when you unmock it it put the orig function you set.

But in fact when you start defining the function the value of *main::stat is undef so this is behaving correctly.

Two issues here:

1. you try to mock a CORE:: function (which can be more tricky)
2. you use define which is blindly defining a function. One of the good practice we established at work is to ban the usage of define (unless you really need it to setup a non existing function) and prefer using redefine. When you use redefine it will die when attempting to mock a non existing function. Which is useful for example when refactoring some code, as you have guarantee to always mock the right thing.

Hope this helps. I think we can close this case. Let me know if you object.

[XSven]

No we cannot close this case:

1. The $mock refers to $class and $class is Bar:: and not main::.
2. I really need define() because I want to inject a wrapped CORE::stat (that is $code) to $class before $class is even loaded (no_auto => 1).
3. I am using the preferred redefine() to mock the wrapped CORE::stat.
4. Calling unmock() should bring back the wrapped CORE::stat that is $code. This doesn't happen and this is the bug.

[atoomic]

If you adjust in your script the line to use redefine

-$mock = Test::MockModule->new( $class, no_auto => 1 )->define( stat => $code );
+$mock = Test::MockModule->new( $class, no_auto => 1 )->redefine( stat => $code );

You would then notice that you are not mocking CORE::stat as you expect but Bar::stat which does not exists so the define silently set a function there, and once removed indeed it does not restore the CORE:: one and put back undef in place to the GV.

> perl test.pl
1..3
Bar::stat does not exist! at test.pl line 14.
BEGIN failed--compilation aborted at test.pl line 17.
# Looks like your test exited with 255 before it could output anything.

I still think you should mock CORE::stat and not Bar::stat

[XSven]

Yes I want to mock CORE::stat but as you have stated mocking a built-in is not that easy. So I have decided to use the recommended approach: I have define()d a usual stat() subroutine that is part of the Bar:: package and that wraps/overrides CORE::stat. This is the purpose of using a define() here:

$mock = Test::MockModule->new( $class, no_auto => 1 )->define( stat => $code );

Now I am able to mock Bar::stat (implicitly CORE::stat) using a redefine(). The unmock() should bring me back a properly functioning Bar::stat (the wrapped/overridden CORE::stat). But this does not happen because the define() implementation is broken because it is based on _mock().

but Bar::stat which does not exists so the define silently set a function there, and once removed indeed it does not restore the CORE:: one and put back undef in place to the GV

I disagree with the indeed...! I don't want to restore CORE::. I want to restore Bar::stat that wraps the CORE::stat and therefore behaves like CORE::stat. define()ing something should never be destoyed by an unmock() action! define() should do this

$self->{_orig}{$name} = \&$sub_name;

By the way the new Sub::Override::inject() (PR13) does exactly what I have expected and while comparing Test::MockModule with Sub::Override I have detected the bug of the define() method.

[XSven]

How do we continue with this issue?

[atoomic]

Sorry for the delay.

The issue is with the example above, you fill the CV for *Bar:stat.

So once compiling the optree, every call to stat in the Bar package is then pointing at &Bar::stat.
Later you remove the definition of &Bar::stat and expect it to fallback to &CORE::stat.

I wonder if we cannot consider something more dynamic to solve your case where when restoring the function if missing we then add the parent or the CORE:: one.

I think your problem is that unmock is retiring a different function via

_replace_sub($sub_name, $self->{_orig}{$name});

when you would expect a different one

[XSven]

Later you remove the definition of &Bar::stat and expect it to fallback to &CORE::stat.

is wrong. Yes, I have defined Bar::stat() using define(). But then I have mocked the defined Bar::stat() using a redefine(). At the end I have unmocked the mock which should bring me back the Bar::stat(). This is not happening. Please read the original case description again. We are moving in circles.

Defining something is not mocking, so define() shouldn't be implemented using _mock().