bigpresh/Dancer-Plugin-Database

t/01-basic.t failure due to hashref stringification problems

ntyni opened this issue · 4 comments

Hi,

as reported in http://bugs.debian.org/665221 we're seeing a test failure in t/01-basic.t:

PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0, 'blib/lib', 'blib/arch')" t/*.t

Testing Dancer::Plugin::Database 1.81, Perl 5.014002, /usr/bin/perl

t/00-load.t ....... ok

Failed test 'database_connection_failed hook fires'

at t/01-basic.t line 155.

got: ''

expected: '1'

Looks like you failed 1 test of 41.

t/01-basic.t ......
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/41 subtests

Testing Dancer::Plugin::Database::Handle 0.12, Perl 5.014002, /usr/bin/perl

t/02-handle.t ..... ok
t/manifest.t ...... skipped: Author tests not required for installation
t/pod-coverage.t .. ok
t/pod.t ........... ok

Test Summary Report

t/01-basic.t (Wstat: 256 Tests: 41 Failed: 1)
Failed test: 41
Non-zero exit status: 1
Files=6, Tests=50, 2 wallclock secs ( 0.07 usr 0.02 sys + 0.68 cusr 0.11 csys = 0.88 CPU)
Result: FAIL
Failed 1/6 test programs. 1/50 subtests failed.
make[1]: *** [test_dynamic] Error 255

This is reproducible for me with 'make test' (but not with 'prove -b t/*.t').

The problem is in using of argument hashref stringification as
a database handle index in Dancer::Plugin::Database::database().
This fails when memory is reused so that a different argument hash gets
the same memory address and hence the same stringification.

Adding this instrumentation into Database.pm:60 or thereabouts:

use Data::Dumper;
my $new = Dumper($handle_key);

if (exists $strings{$handle_key}) {
my $old = $strings{$handle_key};
warn "hashref collision: $old vs. $new" if $old ne $new;
}
$strings{$handle_key} = $new;

gives this output when the test fails:

t/01-basic.t ...... 40/41 hashref collision: $VAR1 = {
'database' => ':memory:',
'driver' => 'SQLite'
};
vs. $VAR1 = {
'dsn' => 'dbi:SQLite:/Please/Tell/Me/This/File/Does/Not/Exist!',
'dbi_params' => {
'HandleError' => sub { "DUMMY" },
'RaiseError' => 0,
'PrintError' => 0
}
};

Failed test 'database_connection_failed hook fires'

at t/01-basic.t line 155.

got: ''

expected: '1'

Looks like you failed 1 test of 41.

which clearly shows the problem: the test is making sure that a
nonexistent database can't be opened, but the code is reusing a handle
for an in-memory database that's working fine.

I'm attaching a test script that should demonstrate this
in a reproducible way.

I suppose the handle index should include a serialization of the
arguments. However, I'm afraid that's hard to do for the general case
as they may include code references (like the 'HandleError' subroutine
above).

Many thanks for your work,

Niko Tyni (Debian Perl Group)
ntyni@debian.org

Wow, sorry about the strange rendering.

I can't see a way to attach files here, so I guess I'll have to cut and paste this.
You can find it as an attachment through the Debian bug report (http://bugs.debian.org/665221).

#!perl

use Test::More tests => 3;
use Dancer::Plugin::Database;

my $opt = { dsn => "dbi:SQLite:dbname=:memory:",
dbi_params => {
HandleError => sub { return 0 }, # gobble connect failed message
RaiseError => 0,
PrintError => 0,
},
};

my $handle;

$handle = database($opt);
ok($handle, "got handle for an in-memory database");

$opt->{dsn} = "dbi:SQLite:dbname=/Please/Tell/Me/This/File/Does/Not/Exist!",
my $opt2;
%$opt2 = %$opt;

$handle = database($opt2);
ok(!$handle, "no handle returned for a nonexistent database");

TODO: {
local $TODO = "broken indexing of cached database handles";
$handle = database($opt);
ok(!$handle, "no cached handle returned for a nonexistent database");
}

Somehow I'd missed this one! Hmm, that's a very good point indeed. I'll have to have a think on the best way to handle that.

dams commented

What you really want is have a reasonably unique signature of the args passed to database(). Instead of using the address of the given hashref, you could also use some of its key / value to build the signature. Maybe concatenate string representations of its content, to a reasonable extend ? like

$signature = join('|', map { join( '-', (@$_ )[0..9] ) } [ keys %$arg ], [ values %$arg ] )

Or something like that. keys and values returning always the same order, that'd work. Or something more clever

This was fixed in 85e8cd0 some time ago, and the bug on bugs.debian.org closed, but looks like I never got round to closing this issue here on GH :)