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

new does not return a new object #48

Open
Altreus opened this issue Aug 5, 2020 · 1 comment
Open

new does not return a new object #48

Altreus opened this issue Aug 5, 2020 · 1 comment

Comments

@Altreus
Copy link

Altreus commented Aug 5, 2020

If you call new twice with the same package name, you get the same object. This makes the entire thing useless.

We have a test library full of helper functions that set up common mocks. In order for this to work at all it must hang onto a reference to the mocked object.

If a test then later tries to mock the same module, it receives the same object! Then, it sets up a mock on that object. It is perfectly reasonable to expect that when that object goes away, the mocked behaviour goes away.

It is not reasonable to expect that the object will not go away after all!

package My::Mock::Helpers;
use Test::MockModule;
my %mocks;

sub mock_external_api_get {
  return if $mocks{mock_external_api};

  my $mock_api = Test::MockModule->new('My::API::Client');
  $mock_api->mock('get_object', sub { return My::API::Client::Object->new_defaults });

  $mocks{mock_external_api} = $mock_api;
}
# test file
use Test::More;
use Test::MockModule;
use Scalar::Util qw(weaken);
use My::DB::Schema;
use My::Mock::Helpers;

My::Mock::Helpers::mock_external_api_get;

subtest "set_object is called" => sub {
  my $mock_api = Test::MockModule->new('My::API::Client'); # NOT MY OBJECT!
  my $mock_api_closure = $mock_api;
  weaken $mock_api_closure;
  $mock_api->mock('set_object', sub {
    # Tests that object 1 is saved
    is $_[1]->id, 1, "Object 1 saved";
    $mock_api_closure->original('set_object')->(@_);
  });
  
  # something that eventually calls set_object
  My::DB::Schema->find_object(id => 1)->save;
};

# ... later ...

My::DB::Schema->find_object(id => 2)->save;

The above example causes a test failure. In the example, My::DB::Schema uses objects that, when save is called on them, send themselves to this example API, by means of My::API::Client.

My::API::Client is also set up to mock GET requests to always return some example default object.

Because Test::MockModule does not return a new object when new is called, the developer who wrote the test mistakenly believes that their subtest will constrain the mocked set_object to the scope of that subref. However, this mock is permanent.

Later in the test, save is called on another object, and the test fails, because a mock that should have gone away has not gone away.

new should always return a new object, so that developers can be confident that it will go out of scope when they think it will.

@atoomic
Copy link
Contributor

atoomic commented Aug 5, 2020

I see your point, you are expecting the local umocked being triggered when the $mock_api is going outside of scope.
Even if internals could decide to share some common object behind.

This is a fair point.

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