Ticket #763 (closed todo: fixed)

Opened 6 years ago

Last modified 5 years ago

Test/More.pir's is_deeply() needs work with hash corner cases

Reported by: Infinoid Owned by:
Priority: normal Milestone:
Component: library Version: 1.2.0
Severity: medium Keywords: test is_deeply pir
Cc: Language:
Patch status: applied Platform:

Description

is_deeply() seems to compare simple hashes just fine.

However, I have some concerns; I can see a couple of things that need to be fixed up.

Issue 1: is_deeply() can't handle hashes containing Undef values, at all. It crashes when you try. I'm attaching a patch which adds a test for that, but currently it crashes.

Issue 2: Once is_deeply() can handle Undefs, it will have some corner cases to work out, with regards to undef values and nonexistent keys. I'm not really sure what the right thing to do is, here. Say one hash is declared with:

%hash1 = (
    key1 => 1,
    key2 => 2,
    key3 => undef,
);

And the other was declared with:

%hash2 = (
    key1 => 1,
    key2 => 2,
);

Should they compare equal? Does %hash2{key3} return an Undef PMC, or PMCNULL?

If they compare equal, then the fix I implemented in r39564 is insufficient. r39564 fixed isues with iterator ordering by removing the right iterator entirely, so it now does the whole comparison based on keys from the left iterator, and relies on the (initial) key-count comparison to ensure no extra keys are in the right hash. But if %hash2{key3} returns an Undef PMC, it will compare successfully with %hash1{key3}. If you add an additional "key4" key to %hash2, it will have 3 in total, thus the hash-count check will also pass and is_deeply() will return a false positive. So an additional pass on the right hash will need to be performed, in order to make sure none of its keys are missing from the left hash (except any that have Undef values).

Either way, we will need further tests to enforce that. And more eyeballs looking for more corner cases too; it would be nice if is_deeply() worked correctly as lots of other code should rely on it.

Attachments

test-is_deeply-undef-vs-nonexist-handling.patch Download (1.3 KB) - added by Infinoid 6 years ago.
test is_deeply()'s undef handling (though I'm not sure what the eventual diagnostic message should say)

Change History

Changed 6 years ago by Infinoid

test is_deeply()'s undef handling (though I'm not sure what the eventual diagnostic message should say)

Changed 6 years ago by Infinoid

  • keywords test is_deeply pir added
  • component changed from none to library
  • type changed from bug to todo
  • patch set to new

Changed 5 years ago by dukeleto

  • status changed from new to closed
  • resolution set to fixed
  • patch changed from new to applied

All of these issues have been taken care of in r41541 and r41542 . %hash1 and %hash2 as detailed above do not evaluate equal, because they have different number of keys. When another key is added so they have the same number of keys, they still do not evaluate equally.

The original patch was a good start, thanks!

Note: See TracTickets for help on using tickets.