Bug #78288 for Test-Deep: Confusing diagnostics

Tue Jul 10 03:52:47 2012 henrik [...] jobindex.dk - Ticket created

Subject:	Confusing diagnostics
Date:	Tue, 10 Jul 2012 09:52:28 +0200
To:	<bug-Test-Deep [...] rt.cpan.org>
From:	Henrik Hald Nørgaard <henrik [...] jobindex.dk>

I am using perl 5, version 14, subversion 2 (v5.14.2) built for x86_64-linux and version 0.110 of Test::Deep. When I run the following oneliner: perl -E 'package Ymer; use Test::Deep; sub isa_ymer { my $class = shift; cmp_deeply($class, isa("Ymer")); } Ymer->isa_ymer();' I get this output: not ok 1 # Failed test at -e line 1. # Checking class of $data with isa() # got : 'Ymer' # expect : 'Ymer' # Tests were run but no plan was declared and done_testing() was not seen. This test fails, because Test::Deep tests if the class "Ymer" is a blessed instance of "Ymer", which is not the case. In Test::Deep version 0.108 this didn't fail. It is okay to change this behaivour, but the diagnostics is confusing, as the values for "got" and "expect" are equal. This is a minor issue. Kind regards Henrik Hald Nørgaard, Programmør Jobindex A/S, Holger Danskes Vej 91, 2000 Frederiksberg Tlf.: +45 38 32 33 55, dir.: +45 38 32 33 81 www.jobindex.dk

Tue Jul 10 04:15:49 2012 fergal [...] esatclear.ie - Correspondence added

Subject:	Re: [rt.cpan.org #78288] Confusing diagnostics
Date:	Tue, 10 Jul 2012 17:15:37 +0900
To:	bug-Test-Deep [...] rt.cpan.org
From:	Fergal Daly <fergal [...] esatclear.ie>

On 10 July 2012 16:52, Henrik Hald Nørgaard via RT <bug-Test-Deep@rt.cpan.org> wrote: Show quoted text

> Tue Jul 10 03:52:47 2012: Request 78288 was acted upon. > Transaction: Ticket created by hhaldn > Queue: Test-Deep > Subject: Confusing diagnostics > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: henrik@jobindex.dk > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=78288 > > > > I am using perl 5, version 14, subversion 2 (v5.14.2) built for > x86_64-linux and version 0.110 of Test::Deep. > > When I run the following oneliner: > > perl -E 'package Ymer; use Test::Deep; sub isa_ymer { my $class = shift; > cmp_deeply($class, isa("Ymer")); } Ymer->isa_ymer();' > > I get this output: > > not ok 1 > # Failed test at -e line 1. > # Checking class of $data with isa() > # got : 'Ymer' > # expect : 'Ymer' > # Tests were run but no plan was declared and done_testing() was not seen. > > This test fails, because Test::Deep tests if the class "Ymer" is a blessed > instance of "Ymer", which is not the case. > > In Test::Deep version 0.108 this didn't fail. It is okay to change this > behaivour, but the diagnostics is confusing, as the values for "got" and > "expect" are equal.

This looks like it happened with this commit https://github.com/rjbs/Test-Deep/commit/5e1a9e26bd203ec285dc5044186e05d320e5fd6b F Show quoted text

> This is a minor issue. > > Kind regards > > Henrik Hald Nørgaard, Programmør > Jobindex A/S, Holger Danskes Vej 91, 2000 Frederiksberg > Tlf.: +45 38 32 33 55, dir.: +45 38 32 33 81 > > www.jobindex.dk > > > >

Tue Jul 10 04:15:50 2012 The RT System itself - Status changed from 'new' to 'open'

Tue Jul 10 13:07:36 2012 ether [...] cpan.org - Correspondence added

RT-Send-CC:

fergal [...] esatclear.ie

The bottom line here is that the "renderVal" logic in Test::Deep is kind of kooky. Early on in the process it seems to drop knowledge of blessed objects and switches to the type of the object itself - by the time renderVal is called, both bless({},'Foo') and 'Foo' come out the same. The necessary changes seem to need to be in Test::Deep::Cmp, but the logic is elusive. Suggestions, Fergal?

Tue Jul 10 13:32:22 2012 ether [...] cpan.org - Correspondence added

RT-Send-CC:

fergal [...] esatclear.ie

On Tue Jul 10 10:07:36 2012, ETHER wrote: Show quoted text

> The bottom line here is that the "renderVal" logic in Test::Deep is kind > of kooky. Early on in the process it seems to drop knowledge of blessed > objects and switches to the type of the object itself - by the time > renderVal is called, both bless({},'Foo') and 'Foo' come out the same. > > The necessary changes seem to need to be in Test::Deep::Cmp, but the > logic is elusive. Suggestions, Fergal?

Fixed: https://github.com/karenetheridge/Test-Deep/tree/topic/blessed_objects_rt78288 https://github.com/rjbs/Test-Deep/pull/6

Tue Jul 10 19:14:20 2012 fergal [...] esatclear.ie - Correspondence added

Subject:	Re: [rt.cpan.org #78288] Confusing diagnostics
Date:	Wed, 11 Jul 2012 08:14:11 +0900
To:	bug-Test-Deep [...] rt.cpan.org
From:	Fergal Daly <fergal [...] esatclear.ie>

Why is it the diags that are the problem? It seems the comparison behaviour changed. Changing comparison behaviour will cause existing test suites to start failing (or worse start passing) when people upgrade. Unless there's a really good reason to change the behaviour, I'd say this should be fixed by adding a case to deal with a bare class name.

Wed Jul 11 03:55:57 2012 henrik [...] jobindex.dk - Correspondence added

Subject:	SV: [rt.cpan.org #78288] Confusing diagnostics
Date:	Wed, 11 Jul 2012 09:55:37 +0200
To:	<bug-Test-Deep [...] rt.cpan.org>
From:	Henrik Hald Nørgaard <henrik [...] jobindex.dk>

Yes, the comparison behavior changed. Yes, that caused our test suite to fail. But the documentation of isa/Isa states that: "This uses UNIVERSAL::isa() to check that $got_v is blessed into the class $class." and I was using isa in a case, where $got_v was not a blessed reference, but a bare class name. So the old version of isa passed more tests, than the documentation promised it would do (and it still does, as it handles e.g. references to a hash). But of course, bare class names now sort of slip through the fingers of Test::Deep::isa... Kind regards Henrik Hald Nørgaard, Programmør Jobindex A/S, Holger Danskes Vej 91, 2000 Frederiksberg Tlf.: +45 38 32 33 55, dir.: +45 38 32 33 81 www.jobindex.dk Show quoted text

-----Oprindelig meddelelse----- Fra: Fergal Daly via RT [mailto:bug-Test-Deep@rt.cpan.org] Sendt: 11. juli 2012 01:14 Til: henrik@jobindex.dk Emne: Re: [rt.cpan.org #78288] Confusing diagnostics <URL: https://rt.cpan.org/Ticket/Display.html?id=78288 > Why is it the diags that are the problem? It seems the comparison behaviour changed. Changing comparison behaviour will cause existing test suites to start failing (or worse start passing) when people upgrade. Unless there's a really good reason to change the behaviour, I'd say this should be fixed by adding a case to deal with a bare class name.

Wed Jul 11 12:42:52 2012 ether [...] cpan.org - Correspondence added

On Tue Jul 10 16:14:20 2012, fergal@esatclear.ie wrote: Show quoted text

> Why is it the diags that are the problem? It seems the comparison > behaviour changed. Changing comparison behaviour will cause existing > test suites to start failing (or worse start passing) when people > upgrade.

The old comparison behaviour was wrong. That's one of the things that v0.109 fixed.

Wed Jul 11 21:22:30 2012 fergal [...] esatclear.ie - Correspondence added

Subject:	Re: [rt.cpan.org #78288] Confusing diagnostics
Date:	Thu, 12 Jul 2012 10:22:20 +0900
To:	bug-Test-Deep [...] rt.cpan.org
From:	Fergal Daly <fergal [...] esatclear.ie>

Show quoted text

> The old comparison behaviour was wrong. That's one of the things that > v0.109 fixed.

Changing long-standing behaviour, even if it was wrong should be signalled clearly. The change log just talks about how the implementation has changed, F

Fri Jul 13 18:19:08 2012 rjbs [...] cpan.org - Correspondence added

On 2012-07-11 12:42:52, ETHER wrote: Show quoted text

> On Tue Jul 10 16:14:20 2012, fergal@esatclear.ie wrote:

> > Why is it the diags that are the problem? It seems the comparison > > behaviour changed. Changing comparison behaviour will cause existing > > test suites to start failing (or worse start passing) when people > > upgrade.

> > The old comparison behaviour was wrong. That's one of the things that > v0.109 fixed.

My guess is that by "wrong," ETHER means "didn't match the documentation." I think the documentation was wrong. (I know, I know, I applied this fix. I even questioned it later, and then did nothing. I think I was confused.) The "isa" test is called "isa." Beyond the fact that this name has caused us grief, it's really good at one thing: calling to mind the universal "isa" method, which behaves in a well-known way. Shouldn't this isa do the same? I think it should and, in fact, that it has until just recently. I think we should revert the change to its behavior in 0.109 and fix its documentation to indicate that it looks for something that answers ->isa for the class, not something blessed into the class. After all, if we wanted to stick with the docs, we'd still be wrong: This uses UNIVERSAL::isa() to check that $got_v is blessed into the class $class. Beyond the fact that it isn't always using UNIVERSAL::isa, it also isn't checking that it's a value blessed into $class. It's checking that it's blessed into $class or a subclass thereof. I do think that many people want to test that their values are instances. For those people, we should add isa_instance or instance. Please let me know your thoughts on this, and I will take action in the next few days. I'll be at OSCON and should have some hacking time. -- rjbs

Fri Jul 13 20:19:59 2012 ether [...] cpan.org - Correspondence added

RT-Send-CC:

fergal [...] esatclear.ie

Show quoted text

> > The old comparison behaviour was wrong. That's one of the things > > that v0.109 fixed.

> > My guess is that by "wrong," ETHER means "didn't match the > documentation." I think the documentation was wrong.

That's not what I meant, but it could be that that is what actually happened. My understanding was that the old behaviour checked that $got_v eq $type OR blessed($got_v) eq $type -- and now we actually check that $got_v is a blessed instance. However, it appears that I am wrong, because: perl -E 'package X{} say X->isa("X")' ...prints true, but (at least in current versions of perl), X->isa("X") is false if there is no X package. Show quoted text

> (I know, I know, I applied this fix. I even questioned it later, and > then did nothing. I think I was confused.)

My bad; I missed an important feature of core isa(). Show quoted text

> The "isa" test is called "isa." Beyond the fact that this name has > caused us grief, it's really > good at one thing: calling to mind the universal "isa" method, which > behaves in a well-known > way. Shouldn't this isa do the same? I think it should and, in fact, > that it has until just > recently.

My expectation would be that Test::Deep::isa() should work like the built-in isa -- which includes allowing subclasses. After all, if the user really wants to check the *exact* type of $got_v, he'd just do: cmp_deeply(blessed($foo), 'X') Show quoted text

> I think we should revert the change to its behavior in 0.109 and fix > its documentation to indicate that it looks for something that answers

->isa Show quoted text

> for the class, not something blessed into the class.

Agreed. Show quoted text

> I do think that many people want to test that their values are > instances. For those people, we > should add isa_instance or instance.

Isn't there a Test::Deep::Blessed? (At least, I recall having to frequently unimport it so it won't conflict with Scalar::Util::blessed..) :) If we want to keep conflicting with builtins (or darned well nearly - Scalar::Util has been cored since before 5.8), that is. It would probably be more friendly to stop exporting blessed() and use a new name, like isa_instance or instance_of. thanks for the clarification, rjbs!

Sat Jul 14 01:53:01 2012 ether [...] cpan.org - Correspondence added

tldr version: should Test::Deep::isa() behave just like Test::More::isa_ok? consider: $ perl -MTest::More -E 'package X{} isa_ok("X", "X"); isa_ok("Y","Y")' ok 1 - The class isa X not ok 2 - The class isa Y $ perl -MTest::More -E 'package A{} package X{@ISA="A"} isa_ok("X", "X"); isa_ok("X", "A")' ok 1 - The class isa X ok 2 - The class isa A

Sat Jul 14 07:07:16 2012 rjbs [...] cpan.org - Correspondence added

On 2012-07-14 01:53:01, ETHER wrote: Show quoted text

> tldr version: should Test::Deep::isa() behave just like Test::More::isa_ok?

Yes, insofar as it should give the answer ->isa gives. As was mentioned on IRC yesterday, this bug with isa has been fixed in bleadperl. -- rjbs

Sat Nov 05 21:41:13 2016 rjbs [...] cpan.org - Correspondence added

The diagnostics are fixed now, too: ~$ perl -E 'package Ymer; use Test::Deep; sub isa_ymer { my $class = shift; Show quoted text

quote> cmp_deeply($class, isa("Ymer")); } Ymer->isa_ymer();'

not ok 1 # Failed test at -e line 2. # Checking class of $data with isa() # got : 'Ymer' # expect : blessed into or ref of type 'Ymer' # Tests were run but no plan was declared and done_testing() was not seen. -- rjbs

Sat Nov 05 21:41:24 2016 rjbs [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #78288 for Test-Deep: Confusing diagnostics

Preferred bug tracker