Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

context level is incorrect when mocks are used #941

Open
karenetheridge opened this issue Oct 7, 2022 · 3 comments
Open

context level is incorrect when mocks are used #941

karenetheridge opened this issue Oct 7, 2022 · 3 comments

Comments

@karenetheridge
Copy link
Member

karenetheridge commented Oct 7, 2022

Here's what I said on irc. I can drill down into my still-pretty-large repro case if you have ideas.. but hopefully something will jump out for you?

15:46 < ether> Exodist: what is the Test2 equivalent of $Test::Builder::Level? I have a helper test module that defines a sub where it is incrementing Level.. and it is being called from a Test2::V0-using test file with a subtest. a failing test in the subtest has the wrong error location - it looks like ...
15:46 < ether> ... Level is 1 less than it should be. (if I increment it twice in the helper sub, the location is correct.) so it seems that Test2's subtest is not incrementing $Level, or the value of $Level is not taking into account whatever value the subtest incremented?
16:18 < leont> It's a context. It's a different model really
21:02 < ether> I thought there was backcompat for this
03:58 < haarg> there is if you're using the Test::Builder API
07:16 < Exodist> If Test::Builder is loaded, then compatibility code is also loaded that makes $Test::Builder::Level work. If Test::Builder is not loaded then the compatibility code is not, and $Test::Builder::Level does not work. Also tests using Test2 are significantly faster when the compatibility code (Test::Builder) is not loaded. But even with Test::Builder loaded Test2 is faster at run-time than old Test::Builder was. Old Test::Builder
07:16 < Exodist> loads a bit faster though. Just to give a very thurough answer
08:55 < ether> Test::Builder is definitely loaded, but I'm seeing a discrepancy in levels.
08:55 < ether> is load order significant somehow?
15:23 < ether> Exodist: hey I have more details on the level issue I described before. I started boiling down my test file to a smaller repro case..
15:23 < ether> the test failure is incorrectly showing up at Test2/Mock.pm line 226..
15:24 < ether> and indeed I am performing a mock in this test file. if I remove the use of mocks, the issue goes away
15:24 < ether> so I conclude that something in the mocking code is messing the level up
15:24 < ether> line 226 is in sub before: $self->_inject({}, $name => sub { $sub->(@_); $orig->(@_) });

@karenetheridge
Copy link
Member Author

more details: I am indeed using a 'before' mock, and the sub being mocked is currently being executed at the time of the test failure. So perhaps it's just as simple as line 226 not incrementing the level when it should. (and it looks like all the other subrefs in this file should do the same.)

@exodist
Copy link
Member

exodist commented Oct 13, 2022

Ah, I think there is a misunderstanding that may need documenting. The mocking tools are intended for mocking/overriding non-test classes, IE the code being overriden is not expected to make any assertions or fire off any test events. The Test2 mock code makes no adjustments to the level, and does not do the Test2 version of that which is acquiring a context.

The mocking cannot assume that the things it mocks are going to fire test events, most do not, and messing with the level/context when assertions will not be made might have undesired side effects (I have to think about that)

If you are using the mocking tools to mock/override things and add assertions or test events inside the mock, you have to take extra action to insure the level/context is ok.

That said, I would expect it to "just work" since the assertion methods (ok, is, etc) should grab the context of the sub they are in, from the mock, which should be the correct file+line, see this example:

use Test2::V0;

my $x = mock {} => (
    add => {
        do_thing => sub {
            ok(0, "Example Failure");
        }   
    }   
);

my $ym = mock 'Some::Object' => (
    add => {
        do_thing => sub {
            ok(0, "Example Failure");
        }   
    }   
);

my $y = bless {}, 'Some::Object';

$x->do_thing;
$y->do_thing;

done_testing;

Output:

perl test.t
# Seeded srand with seed '20221013' from local date.
not ok 1 - Example Failure
# Failed test 'Example Failure'
# at test.t line 6.
not ok 2 - Example Failure
# Failed test 'Example Failure'
# at test.t line 14.
1..2

As seen, the file+line number are correct. I also tried swapping out Test2::V0 with Test::More and added a use Test2::Tools::Mock so using/loading Test::Builder tools does not throw it off. It worked fine.

I think I will need a more complete example of the mocks messing with the line numbers in order to debug your issue any further. Can you build a reproduction case?

@karenetheridge
Copy link
Member Author

Here is a repro case:

use strict;
use warnings;
use Test2::V0;

{
    package TestHelper;
    use strict;
    use warnings;
    use Test::More;

    sub my_test {
        local $Test::Builder::Level = $Test::Builder::Level + 1;
        Test::More::fail 'this is a failing test';
        Test::More::ok(0, 'another failing test');
    }
}

# if the mock is commented out, the failure locations are correct
my $mock = mock 'TestHelper' => ( before => [ my_test => sub { } ] );

fail 'failing test';
TestHelper::my_test;    # location should give this line number, not the sub

done_testing;

The reason I am doing a 'before' mock in a test helper is I am clearing a state variable before each "grouping" of tests (that is, my_test prepares an HTTP request, sends it to the test listener, gathers the response, tests various things about it, etc...) and the state variable is holding a counter of certain events that happen during the http request, which are then tested for.

@exodist exodist transferred this issue from Test-More/Test2-Suite Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants