Skip to content

Commit

Permalink
Remove cache_aware_c and mock caches
Browse files Browse the repository at this point in the history 
Initialize the Redis cache/store in the normal `c` instead of requiring access
to a separate `cache_aware_c` in the tests. Remove the mock caches, and just
use the Redis caches, which is a better test of real code; in fact it revealed
at least one bug where we don't invalidate releases from the cache when their
ASINs are updated.

I didn't see any point to the `EntityCache` and `GIDEntityCache` tests having
to check how many times certain mock methods were called. It had no bearing on
what ended up being stored in the cache, which was all that mattered.
  • Loading branch information
@mwiencek
mwiencek committed Jan 16, 2024
1 parent d93ca6c commit f981f2d
Show file tree
Hide file tree
Showing 13 changed files with 109 additions and 298 deletions.
8 changes: 7 additions & 1 deletion lib/MusicBrainz/Server.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,13 @@ unless (DBDefs->CATALYST_DEBUG) {
push @args, 'ErrorInfo';
}

__PACKAGE__->config->{'Plugin::Cache'}{backend} = DBDefs->PLUGIN_CACHE_OPTIONS;
{
my $plugin_cache_opts = DBDefs->PLUGIN_CACHE_OPTIONS;
if ($ENV{MUSICBRAINZ_RUNNING_TESTS}) {
$plugin_cache_opts->{database} = DBDefs->REDIS_TEST_DATABASE;
}
__PACKAGE__->config->{'Plugin::Cache'}{backend} = $plugin_cache_opts;
}

require MusicBrainz::Server::Authentication::WS::Credential;
require MusicBrainz::Server::Authentication::WS::Store;
Expand Down
2 changes: 2 additions & 0 deletions lib/MusicBrainz/Server/Data/Release.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1714,6 +1714,8 @@ sub update_amazon_asin {
{ id => $release->id },
);
}

$self->_delete_from_cache($release_id);
}

__PACKAGE__->meta->make_immutable;
Expand Down
2 changes: 1 addition & 1 deletion lib/MusicBrainz/Server/Model/MB.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ sub _build_context {

if ($ENV{MUSICBRAINZ_RUNNING_TESTS}) {
require MusicBrainz::Server::Test;
return MusicBrainz::Server::Test->create_test_context;
return MusicBrainz::Server::Test->get_test_context;
} else {
my $cache_opts = DBDefs->CACHE_MANAGER_OPTIONS;
my $c = MusicBrainz::Server::Context->new(
Expand Down
37 changes: 21 additions & 16 deletions lib/MusicBrainz/Server/Test.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,27 +66,32 @@ MusicBrainz::Server::DatabaseConnectionFactory->alias('READWRITE' => 'TEST');
our $test_context;
our $test_transport = Email::Sender::Transport::Test->new();

sub get_test_context
{
my ($class) = @_;

$test_context //= $class->create_test_context;
return $test_context;
}

sub create_test_context
{
my ($class, %args) = @_;

$test_context ||= do {
my $cache_manager = MusicBrainz::Server::CacheManager->new(
profiles => {
null => {
class => 'Cache::Null',
wrapped => 1,
},
},
default_profile => 'null',
);
MusicBrainz::Server::Context->new(
cache_manager => $cache_manager,
%args,
);
};
my $cache_opts = DBDefs->CACHE_MANAGER_OPTIONS;
my $store_opts = DBDefs->DATASTORE_REDIS_ARGS;

return $test_context;
$cache_opts->{profiles}{external}{options}{database} =
DBDefs->REDIS_TEST_DATABASE;
$store_opts->{database} =
DBDefs->REDIS_TEST_DATABASE;

return MusicBrainz::Server::Context->new(
cache_manager =>
MusicBrainz::Server::CacheManager->new(%$cache_opts),
store => MusicBrainz::DataStore::Redis->new(%$store_opts),
%args,
);
}

sub _load_query
Expand Down
45 changes: 3 additions & 42 deletions t/lib/t/Context.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,44 +11,11 @@ use MusicBrainz::Server::Test;
has c => (
is => 'ro',
builder => '_build_context',
);

has cache_aware_c => (
is => 'ro',
builder => '_build_cache_aware_context',
clearer => '_clear_cache_aware_c',
predicate => '_has_cache_aware_c',
lazy => 1,
);

sub _build_context {
MusicBrainz::Server::Test->create_test_context();
}

sub _build_cache_manager {
my $test = shift;

my $opts = DBDefs->CACHE_MANAGER_OPTIONS;
$opts->{profiles}{external}{options}{database} = DBDefs->REDIS_TEST_DATABASE;
return MusicBrainz::Server::CacheManager->new(%$opts);
}

sub _build_store {
my $opts = DBDefs->DATASTORE_REDIS_ARGS;
$opts->{database} = DBDefs->REDIS_TEST_DATABASE;
return MusicBrainz::DataStore::Redis->new(%$opts);
}

sub _build_cache_aware_context {
my $test = shift;

return $test->c->meta->clone_object(
$test->c,
cache_manager => $test->_build_cache_manager,
store => $test->_build_store,
models => {}, # Need to reload models to use this new $c
fresh_connector => 1,
);
MusicBrainz::Server::Test->get_test_context;
}

around run_test => sub {
Expand All @@ -59,8 +26,6 @@ around run_test => sub {

my $c = $self->c;

$c->connector->_disconnect;

$c->sql->begin;

$self->$orig(@_);
Expand All @@ -71,12 +36,8 @@ around run_test => sub {
}

$c->sql->rollback;

if ($self->_has_cache_aware_c) {
my $cache_aware_c = $self->cache_aware_c;
$cache_aware_c->cache->clear;
$cache_aware_c->store->clear;
}
$c->cache->clear;
$c->store->clear;
};

1;
2 changes: 1 addition & 1 deletion t/lib/t/MusicBrainz/Server/Data/Alias.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ test 'Exists only checks a single entity' => sub {

test 'Modifying instrument aliases invalidates the link attribute type caches' => sub {
my $test = shift;
my $c = $test->cache_aware_c;
my $c = $test->c;

my $piano_lat_id = 180;
my $piano_via_all;
Expand Down
2 changes: 1 addition & 1 deletion t/lib/t/MusicBrainz/Server/Data/Artist.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ $sql->commit;
test 'Merging with a cache' => sub {
my $test = shift;

my $c = $test->cache_aware_c;
my $c = $test->c;
my $cache = $c->cache_manager->_get_cache('external');

MusicBrainz::Server::Test->prepare_test_database($c, '+data_artist');
Expand Down
2 changes: 1 addition & 1 deletion t/lib/t/MusicBrainz/Server/Data/ArtistCredit.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ test 'Merging updates matching names' => sub {

test 'Merging clears the cache' => sub {
my $test = shift;
my $c = $test->cache_aware_c;
my $c = $test->c;
my $cache = $c->cache_manager->_get_cache('external');
my $artist_credit_data = $c->model('ArtistCredit');

Expand Down
131 changes: 31 additions & 100 deletions t/lib/t/MusicBrainz/Server/Data/EntityCache.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,110 +5,41 @@ use warnings;
use Test::Routine;
use Test::Moose;
use Test::More;

{
package MyEntityData;
use Moose;
use namespace::autoclean;

extends 'MusicBrainz::Server::Data::Entity';
sub _type { 'my_entity_data' }
sub get_by_ids
{
my $self = shift;
$self->get_called(1);
return { 1 => 'data' };
}

package MyCachedEntityData;
use Moose;
use namespace::autoclean;

extends 'MyEntityData';
with 'MusicBrainz::Server::Data::Role::EntityCache';
has 'get_called' => ( is => 'rw', isa => 'Bool', default => 0 );

package MockCache;
use Moose;
use namespace::autoclean;

has 'data' => ( is => 'rw', isa => 'HashRef', default => sub { +{} } );
has 'get_called' => ( is => 'rw', isa => 'Bool', default => 0 );
has 'set_called' => ( is => 'rw', isa => 'Bool', default => 0 );
sub get
{
my ($self, $key) = @_;
$self->get_called(1);
return $self->data->{$key};
}
sub set
{
my ($self, $key, $data) = @_;
$self->set_called(1);
$self->data->{$key} = $data;
}
}

use MusicBrainz::Server::CacheManager;
use MusicBrainz::Server::Context;

with 't::Context' => { -excludes => '_build_context' };

sub _build_context {
my $cache_manager = MusicBrainz::Server::CacheManager->new(
profiles => {
test => {
class => 'MockCache',
wrapped => 1,
keys => ['my_entity_data'],
},
},
default_profile => 'test',
);

return MusicBrainz::Server::Context->new(cache_manager => $cache_manager);
}
with 't::Context';

test all => sub {

my $test = shift;
my $c = $test->c;
my $sql = $c->sql;
my $entity_data = MyCachedEntityData->new(c => $c);

$sql->begin;
my $entity = $entity_data->get_by_id(1);
$sql->commit;
is ( $entity, 'data' );
is ( $entity_data->get_called, 1 );
is ( $test->c->cache->_orig->get_called, 1 );
is ( $test->c->cache->_orig->set_called, 1 );
ok ( $test->c->cache->_orig->data->{'my_entity_data:1'} =~ 'data' );


$entity_data->get_called(0);
$test->c->cache->_orig->get_called(0);
$test->c->cache->_orig->set_called(0);

$sql->begin;
$entity = $entity_data->get_by_id(1);
$sql->commit;
is ( $entity, 'data' );
is ( $entity_data->get_called, 0 );
is ( $test->c->cache->_orig->get_called, 1 );
is ( $test->c->cache->_orig->set_called, 0 );


delete $test->c->cache->_orig->data->{'my_entity_data:1'};
$sql->begin;
$entity = $entity_data->get_by_id(1);
$sql->commit;
is ( $entity, 'data' );
is ( $entity_data->get_called, 1 );
is ( $test->c->cache->_orig->get_called, 1 );
is ( $test->c->cache->_orig->set_called, 1 );
ok ( $test->c->cache->_orig->data->{'my_entity_data:1'} =~ 'data' );

my $test = shift;
my $c = $test->c;
my $sql = $c->sql;
my $cache = $c->cache('artist');
my $artist_data = $c->model('Artist');

$sql->auto_commit(1);
$sql->do(<<~'SQL');
INSERT INTO artist (id, gid, name, sort_name)
VALUES (3, 'e7717242-d43f-46e0-b5ef-9a46ca4d458a', 'Test', 'Test');
SQL

ok(!$cache->exists('artist:3'),
'artist is not in the cache');

$sql->begin;
my $artist = $artist_data->get_by_id(3);
$sql->commit;

is($artist->id, 3,
'get_by_id returns artist with id=3 before caching');
ok($cache->get('artist:3')->isa('MusicBrainz::Server::Entity::Artist'),
'cache contains artist for id');

$sql->begin;
$artist = $artist_data->get_by_id(3);
$sql->commit;

is($artist->id, 3,
'get_by_id returns artist with id=3 after caching');
};

1;
2 changes: 1 addition & 1 deletion t/lib/t/MusicBrainz/Server/Data/Instrument.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ test 'Load basic data' => sub {

test 'Create, update, delete instruments' => sub {
my $test = shift;
my $c = $test->cache_aware_c;
my $c = $test->c;

MusicBrainz::Server::Test->prepare_test_database($c, '+data_instrument');

Expand Down
2 changes: 1 addition & 1 deletion t/lib/t/MusicBrainz/Server/Data/LinkAttributeType.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ test 'get_by_gid with non existent GID' => sub {

test 'Updating a link attribute invalidates cache entries for links' => sub {
my $test = shift;
my $c = $test->cache_aware_c;
my $c = $test->c;

$c->sql->do(<<~'SQL');
INSERT INTO link (id, link_type, attribute_count)
Expand Down

0 comments on commit f981f2d

Please sign in to comment.