$CLASS mock within DESCRIBE() affects other tests

[hbmaclean]

If $CLASS methods are overridden within a describe() block, the actual class methods may still be overridden for other tests within the .t. This is particularly nasty because the problem depends on execution order, and the test at cause is not the one that fails. Prior to recognizing this pattern (mock within describe), I fruitlessly triaged many "broken" tests, only to have them "self-correct" over time.

If this is intended behavior, it should be very well documented.

Mnemonic: DESCRIBE [D]oesn't [E]ffectively [S]cope [C]lass, [R]esidual [I]nterference [B]eyond [E]nclosure.

Workaround is to undef $class_mock, presumably in an after_each.

May relate to #189.

Demonstration:

=head1 SYNOPSIS

Assumption is that each tests() and describe() block is fully
enclosed and independent from all other blocks.

However, below demonstrates that a $CLASS mock within a describe()
block continues to affect subsequent tests!

Presumably, running this without --shuffle and without a specific seed
should demonstrate the problem - the second test should fail.

 prove -v Test2DescribeBug.t
 
If necessary, determine a seed value to force the order:

 T2_RAND_SEED=1 prove -v Test2DescribeBug.t

=cut

use Test2::V0 -target => 'Data::Dumper';
use Test2::Tools::Spec;

my @values = (1, 1, 2, 3, 5, 8, 13);
my $dumper = Data::Dumper->new(@͏values);

=head2 test_mocked

Silly case-based test to demonstrate the problem.

$CLASS->Values() is mocked to return nothing; and two
stubbed cases trigger the test.

Both "cases" should always pass.

=cut

describe test_mocked => sub {
    my $class_mock;

    before_case setup => sub {
        $class_mock = mock($CLASS => ( override => [
            Values => sub {},  # return nothing in this block
        ]));
    };

    # no-op cases, to trigger the test execution
    case case_one => sub {};
    case case_two => sub {};

    after_case teardown => sub {
        # workaround - explicitly undef mock
        # uncomment to demonstrate
        #undef $class_mock;
    };

    tests test_mocked => sub {
        my @got      = $dumper->Values();
        my $expected = [];
        is(
            @got,
            $expected,
            'Values() overridden in describe block'
        );
    };
};

=head2 test_unmocked

Normally, this would be a tests() block, but that would
likely execute after describe(), unless shuffled with a
specific seed.  For the demonstration it is wrapped in
describe() to increase the likelihood of top-down execution.

True expectation is that $CLASS->Values() is *not* mocked;
but in this scenario, this test should fail; and then work
again if $class_mock is undef'd in the earlier test.

=cut

describe test_unmocked => sub {
    tests test_unmocked => sub {
        my @got      = $dumper->Values();
        my $expected = @values;
        is(
            @got,
            $expected,
            'Values() *not* overridden by other test'
        );
    };
};

done_testing();

[exodist]

So, the cause here is that $class_mock is closed-over by the subs you define inside the describe block. for instance the 'test_mocked' test sub. These subs do not get destroyed as they are kept in a structure inside the framework. This has to be this way because of the way the framework defines the blocks at the start, then runs them later. Also the blocks may be reused in other places, we do not know when they are no longer needed.

Since the variable never gets garbage collected it is never destroyed and thus the mock never ends. This is not really a design decision of the framework, and also not something the framework can do anything about. This is a perl behavior, if you enclose a variable you have to be aware of when/where it may or may not be destroyed. Your "fix" of using an after_all to clear the variable is correct.

Hmm, there is also a behavior that I can not find documented, which is a huge oversight. If mock is used in a void context inside a describe block, it will act like a before_each:

use Test2::Tools::Spec;
use Test2::V0;

describe foo => sub {
    mock 'Some::Class' => (
        add => [foo => sub { 'foo' }], 
    );  

    tests foo => sub {
        is(Some::Class->foo, 'foo', "Mock worked");
    };  
};

tests foo => sub {
    ok(!Some::Class->can('foo'), "No Mock");
};

done_testing;

^ that test works fine.

The behavior you have reported is "works as expected" and is the way perl works, not really a framework thing to address, except by documentation.

So this ticket has 2 action items for me:

1. There should be documentation in both the mocking tools, and the spec module that mentions this caveat of closing over a mock object. Most seasoned perl dev's would spot the issue and cause, but a lot would not. It should be called out.
2. The docs for both should also mention that there is already functionality baked in to prevent anyone from doing this, the fact that mock() in void context inside a describe block acts like a before-each so that you can use it as a before-each without worrying about destroying the mock object. (code is here: https://metacpan.org/dist/Test2-Suite/source/lib/Test2/Tools/Spec.pm#L69)

We may also want to consider ways to apply mocks as things other than 'before_each' as in your example you are using this as a before_case, there is no current way to do that apart from your way where you manually destroy the object.

[hbmaclean]

Thanks for the quick, thorough explanation and void workaround. Prominent documentation should go a long way.

[tobyink]

Also the blocks may be reused in other places, we do not know when they are no longer needed.

True, but whether they're still needed or not, we can weaken our reference to them once we no longer need them. Right?

[exodist]

No. When I say the blocks may be used in other places, I mean within the framework itself. Specifically the framework often inspects the coderefs to get their line numbers and filenames. Also you can compose workflows from other workflows that may or may not have already run. We can make no assumptions about when the blocks are no longer needed.