perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

t/test.pl: Handle displaying of read-only items

Open khwilliamson opened this issue 1 year ago • 5 comments

The function display() prettifies its input for display for better human consumption. But it does this by actually modifiying the input, so will crash when passed a read-only value, The simple solution is to make a copy of the input, and modify that instead.

I'm somewhat surprised this hasn't come up before. A simple

like("A", qr/B/)

triggers it.

khwilliamson avatar Aug 24 '24 15:08 khwilliamson

The function display() prettifies its input for display for better human consumption. But it does this by actually modifiying the input, so will crash when passed a read-only value, The simple solution is to make a copy of the input, and modify that instead.

I'm somewhat surprised this hasn't come up before. A simple

like("A", qr/B/)

triggers it.

Is there any way we can construct a regression test for t/test.pl itself?

I tried to create one that put that example in an eval { }; and looked to see that $@ did not contain Modification of a read-only value attempted. But it appears that this bug only gets triggered with a failure of test.pl's like.

jkeenan avatar Aug 24 '24 18:08 jkeenan

I haven't checked, but I suspect the sub is only called upon failure. You can use whichever of unlike or like to cause any particular test to fail. I hadn't thought of using eval, but that sounds like it could work. Where to put it though? A new test file?

khwilliamson avatar Aug 24 '24 19:08 khwilliamson

I haven't checked, but I suspect the sub is only called upon failure. You can use whichever of unlike or like to cause any particular test to fail. I hadn't thought of using eval, but that sounds like it could work. Where to put it though? A new test file?

In principle, it could go into any test file that already has require ./test.pl. Here's my (unsatisfactory) attempt at putting it into a new file.

$ cat t/re/gh-22537-1.t 
#!./perl

BEGIN {
    chdir 't' if -d 't';
    require './test.pl';
    set_up_inc('.', '../lib');
}

plan (2);

{
    local $@;
    eval { like("A", qr/B/, 'message'); };
    unlike($@,
        qr/^Modification of a read-only value attempted/,
        "t/test.pl did not attempt to modify a read-only value"
    );
}

Running:

$ cd t;./perl harness -v re/gh-22537-1.t; cd -

not ok 1 - message
ok 2 - t/test.pl did not attempt to modify a read-only value
# Failed test 1 - message at re/gh-22537-1.t line 13
#      got 'A'
# expected /(?^:B)/
Failed 1/2 subtests 

Test Summary Report
-------------------
re/gh-22537-1.t (Wstat: 0 Tests: 2 Failed: 1)
  Failed test:  1
Files=1, Tests=2,  0 wallclock secs ( 0.01 usr +  0.00 sys =  0.01 CPU)
Result: FAIL

The eval-ed test's failure causes the file as a whole to FAIL, even though we demonstrate that we have avoided the exception.

jkeenan avatar Aug 24 '24 19:08 jkeenan

How about a string eval

khwilliamson avatar Aug 24 '24 19:08 khwilliamson

I haven't checked, but I suspect the sub is only called upon failure. You can use whichever of unlike or like to cause any particular test to fail. I hadn't thought of using eval, but that sounds like it could work. Where to put it though? A new test file?

You could just test the display() function, which is what you modify here.

There's already test.pl specific tests in t/test_pl/

tonycoz avatar Aug 28 '24 01:08 tonycoz

I didn't spend much time on it, but I couldn't get plain display to fail. So, I put this in a new like_unlike.t file in the directory @tonycoz suggested.

khwilliamson avatar Aug 30 '24 16:08 khwilliamson

When I build your branch up through make test_prep, and then run this program from the command-line, I get a warning:

 ./perl -Ilib t/test_pl/like_unlike.t 
test.pl had problems loading Config: Can't locate Config.pm in @INC (you may need to install the Config module) (@INC entries checked: lib /usr/local/lib/perl5/site_perl/5.41.4/x86_64-linux /usr/local/lib/perl5/site_perl/5.41.4 /usr/local/lib/perl5/5.41.4/x86_64-linux /usr/local/lib/perl5/5.41.4) at ./test.pl line 971.
ok 1 - Verify test completed
1..1

Can this be revised such that no warning is thrown? Thanks.

jkeenan avatar Aug 31 '24 21:08 jkeenan

May I suggest replacing the test file by this?

diff --git t/test_pl/display_ro.t t/test_pl/display_ro.t
new file mode 100644
index 0000000000..49e4c5b6c5
--- /dev/null
+++ t/test_pl/display_ro.t
@@ -0,0 +1,13 @@
+#!./perl
+
+BEGIN {
+    chdir 't' if -d 't';
+    require './test.pl';
+}
+
+# This test used to die. See GH #22537.
+my $t = eval { display("ABC \x{20AC}") };
+is $@, '', "display() doesn't die on read-only strings";
+is $t, 'ABC \\x{20ac}', 'display() escapes Unicode characters correctly';
+
+done_testing();

It tests display() directly without having to spawn a fresh interpreter.

mauke avatar Sep 01 '24 20:09 mauke

Using the suggestion by @mauke

khwilliamson avatar Sep 01 '24 21:09 khwilliamson