DrHyde / perl-modules-Number-Phone

Number::Phone and friends
24 stars 32 forks source link

Don't test internal state of string #100

Closed Grinnz closed 3 years ago

Grinnz commented 3 years ago

Test::utf8 is used here to test whether a string returned by the module is upgraded. This is unnecessary and improper. From the original commit (https://github.com/DrHyde/perl-modules-Number-Phone/commit/5305eac06d55d4611a5d730585a0d08049ed1031) it looks like the intent was to test whether the string was returned as decoded Unicode characters, which can be tested with a simple string equality test against the properly decoded string that is expected.

The utf8 flag test is improper for this because it is an implementation detail which is only part of the story. If the string is decoded and then downgraded, it will still contain the correct Unicode characters to any pure-perl code (assuming all of the characters are below U+FF so it can be stored downgraded), so the test failing is incorrect in that case. If the string is not decoded to characters, it can still be upgraded by Perl operations arbitrarily, which will fool this test.

I don't know the context to tell whether an exact string match is an appropriate test here. There's no generic way to test whether a string is decoded characters, because Perl strings contain whatever they are interpreted to contain.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 99.447% when pulling f2e26908dca738098dfbeb91f6ed388b9e91c57f on Grinnz:no_utf8_test into 41be93cce4ba327bc05a79f0f511b0e7d6225e41 on DrHyde:master.

DrHyde commented 3 years ago

Thanks!