[pm-h] Adventures In Getting Bitten By a Closure - Is there a better way to address this issue?

[Julian Brown]

First of I love Test::MockModule,

I am wondering if you run this on different versions of Perl if your
results would be different and possibly a bug in Perl for that version?

Julian

On Fri, Dec 16, 2016 at 12:43 PM, Robert Stone via Houston <houston at pm.org>
wrote:

> Greetings,
>
>
> tl;dr - refcount unexpectedly incremented, caused DESTROY method on object
> to never get called.  Used a workaround to address but perhaps there is a
> cleaner way?
>
>
> Jocelyn and I just spent about 3 hours tracking down why a DESTROY wasn't
> being called, a whole bunch of refcounts later we found the problem.  I'm
> only fairly confident in my analysis below and would love it for any of you
> to chime in and say "Yep!  Sounds about right" or "No way, what actually
> happened was..."  Should be good times for all of us.
>
> For those of you who aren't fully versed on the magic that is
> Test::MockModule, it's the greatest thing since the circumflexion
> operator.  It allows you to mock a method of an object and then when the
> variable that you assigned Test::MockModule->new to goes out of scope then
> the mock is removed.  In general, very handy and it works perfectly.
>
> Here is the subroutine I was using to define and install my mock:
>
>   sub restrict_search_space {
>       my ( $user_ids ) = pos_validated_list( \@_, { isa => 'ArrayRef[Int]'
> } );
>
>       my $mocked_user = Test::MockModule->new(
> 'FreeChat::API::Model::User' );
>       $mocked_user->mock(
>           'search',
>           sub {
>               my $self       = shift;
>               my $condition  = shift;
>               my $attributes = shift // {};
>
>               $condition->{'me.id'} = { 'in' => $user_ids };
>               return $mocked_user->original( 'search' )->( $self,
> $condition, $attributes );
>           } );
>
>       return $mocked_user;
>   }
>
> Seems easy enough right?  I create an instance of Test::MockModule that
> mocks FreeChat::API::Model::User.  Then I mock the search method so that I
> can restrict the search space by adding an additional condition to the
> WHERE clause that is ultimately passed down to DBIx's search method.
> Because what I really want here is an AROUND instead of a straight up mock
> I end up having to call $mocked_user->original('search') to get the
> original unmocked method, thereby making this into a sort of AROUND.
>
> This works no problem until you try to do something like this (actual
> tests!  Names were not changed to protect the guilty):
>
>       subtest 'Gender' => sub {
>           my ( $mech, $user ) = get_mech_and_user();
>
>           my $male_user   = create_user( gender => 'Male' );
>           my $female_user = create_user( gender => 'Female' );
>
>           my $mocked_user = restrict_search_space( [ $male_user->id,
> $female_user->id ] );
>
>           $mech->get_ok( '/users?gender=Male' );
>
>           note( 'Raw JSON Response: ' . $mech->content );
>           my $json = from_json( $mech->content );
>
>           ok( ( grep { $_->{id} == $male_user->id } @{$json} ), 'Matching
> user correctly identified' );
>           ok( !( grep { $_->{id} == $female_user->id } @{$json} ),
> 'Unmatching user correctly not identified' );
>       };
>
>       subtest 'Status' => sub {
>           my ( $mech, $user ) = get_mech_and_user();
>
>           my $active_user  = create_user( status => 'Active' );
>           my $offline_user = create_user( status => 'Offline' );
>
>           my $mocked_user = restrict_search_space( [ $active_user->id,
> $offline_user->id ] );
>
>           $mech->get_ok( '/users?status=Active' );
>
>           note( 'Raw JSON Response: ' . $mech->content );
>           my $json = from_json( $mech->content );
>
>           cmp_ok( $mech->status, '==', 200, 'Correct status' );
>
>           ok( ( grep { $_->{id} == $active_user->id } @{$json} ),
> 'Matching user correctly identified' );
>           ok( !( grep { $_->{id} == $offline_user->id } @{$json} ),
> 'Unmatching user correctly not identified' );
>       };
>
> The first subtest passes no problem, the second one though does not.  Each
> one does pass in isolation, but run together they fail.  It fails because
> the instance of Test::MockModule never goes out of scope (the object
> pointed to by $mocked_user has a refcount of 2, not of 1) and therefore the
> mock never gets uninstalled.  CURSES!
>
> See the issue?
>
>       my $mocked_user = Test::MockModule->new(
> 'FreeChat::API::Model::User' );
>       $mocked_user->mock(
>           'search',
>           sub {
>               my $self       = shift;
>               my $condition  = shift;
>               my $attributes = shift // {};
>
>               $condition->{'me.id'} = { 'in' => $user_ids };
>               *return $mocked_user->original( 'search' )->( $self,
> $condition, $attributes );*
>           } );
>
> It seems that because I called $mocked_user here and have the entire thing
> wrapped in a sub, I'm creating a closure causing the refcount on
> $mocked_user to be incremented.  It seems that one of the references comes
> from restrict_search_space's $mocked_user and the other comes from the
> subtest's $mocked_user.  When I make the below modification:
>
>   sub restrict_search_space {
>       my ( $user_ids ) = pos_validated_list( \@_, { isa => 'ArrayRef[Int]'
> } );
>
>       my $mocked_user = Test::MockModule->new(
> 'FreeChat::API::Model::User' );
>       *my $original_method;*
>       $mocked_user->mock(
>           'search',
>           sub {
>               my $self       = shift;
>               my $condition  = shift;
>               my $attributes = shift // {};
>
>               $condition->{'me.id'} = { 'in' => $user_ids };
>               return *$original_method*->( $self, $condition, $attributes
> );
>           } );
>
>       *$original_method = $mocked_user->original( 'search' );*
>
>       return $mocked_user;
>   }
>
> Everything worked as expected, at the end of the subtest the $mocked_user
> goes out of scope and the method gets unmocked.
>
> The real question, while this works it seems awfully hacky and ugly.  Is
> there a better way to do this?  Should I have used weaken somewhere?
>
> Just thought you all would enjoy commiserating with us.  Hope everyone is
> having a safe and happy holiday.
>
> Best Regards,
> Robert Stone
>
> _______________________________________________
> Houston mailing list
> Houston at pm.org
> http://mail.pm.org/mailman/listinfo/houston
> Website: http://houston.pm.org/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.pm.org/pipermail/houston/attachments/20161216/05e55d83/attachment.html>