New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test::More::skip() failures in master #153

Closed
ribasushi opened this Issue Jun 7, 2016 · 12 comments

Comments

Projects
None yet
2 participants
@ribasushi
Contributor

ribasushi commented Jun 7, 2016

Filing this as per #perl11, sorry if this is a known issue.

Failure within Package::Stash::XS:

skip() needs to know $how_many tests are in the block at t/00-compile.t line 66
# Looks like you planned 1 test but ran 2.
[16:16:35] t/00-compile.t ........... 
Dubious, test returned 255 (wstat 65280, 0xff00)
All 1 subtests passed 
    (less 1 skipped subtest: 0 okay)
Redundant argument in sprintf at t/addsub.t line 33.

Failure within DBI:

[16:16:44] t/48dbi_dbd_sqlengine.t ......... ok       78 ms ( 0.00 usr  0.00 sys +  0.07 cusr  0.01 csys =  0.08 CPU)
Not enough arguments for subroutine entry Test::More::skip. Missing $why at t/49dbd_file.t line 83, near "skip;"
Not enough arguments for subroutine entry Test::More::skip. Missing $why at t/49dbd_file.t line 88, near "skip;"
Execution of t/49dbd_file.t aborted due to compilation errors.
[16:16:44] t/49dbd_file.t .................. 
Dubious, test returned 255 (wstat 65280, 0xff00)
@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 7, 2016

Contributor

IMPORTANT

The newfangled Test::More no longer requires neither reason nor count. Therefore it may be misguided to ask authors of e.g. DBI to fix their bare-skip's, and instead merge the 5.25.x blead Test::More offering...

rabbit@Ahasver:~$ perl -MTest::More\ 999
Test::More version 999 required--this is only version 1.302026.
BEGIN failed--compilation aborted.
rabbit@Ahasver:~$ perl -e 'use Test::More; SKIP: { skip "foo", 1; }; done_testing'; echo $?
ok 1 # skip foo
1..1
0
rabbit@Ahasver:~$ perl -e 'use Test::More; SKIP: { skip; }; done_testing'; echo $?
ok 1 # skip
1..1
0
Contributor

ribasushi commented Jun 7, 2016

IMPORTANT

The newfangled Test::More no longer requires neither reason nor count. Therefore it may be misguided to ask authors of e.g. DBI to fix their bare-skip's, and instead merge the 5.25.x blead Test::More offering...

rabbit@Ahasver:~$ perl -MTest::More\ 999
Test::More version 999 required--this is only version 1.302026.
BEGIN failed--compilation aborted.
rabbit@Ahasver:~$ perl -e 'use Test::More; SKIP: { skip "foo", 1; }; done_testing'; echo $?
ok 1 # skip foo
1..1
0
rabbit@Ahasver:~$ perl -e 'use Test::More; SKIP: { skip; }; done_testing'; echo $?
ok 1 # skip
1..1
0
@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Jun 8, 2016

Member

That's true.
I'll make the args optional but keep the strict types.

This is now a FAQ:

So that when you use one or two args for Test::More::skip(), they need to properly typed.

I.e.

skip $why, 1` => `skip "$why", 1
skip "why", scalar(@skips) => skip "why", int(@skips)
skip "why", 2*$skips => skip "why", int(2*$skips)

Rationale:

skip() is a special case that the two args are very often mixed up. This error had previously to be detected at run-time with a fragile \d regex check. And in most cases this problem was never fixed, e.g. in Module::Build.
Only with checking for strict types I could detect and fix all of the wrong usages, get rid of the unneeded run-time check, the code is better, all errors are detected at compile time, and not covered at run-time and with the new strict types the code is much more readable, what is the Str $why and what the UInt $count argument.

$count can never be a NV, thus :Numeric (which would allow 2*$skip) is wrong. It needs to be :UInt. The used range op (pp_flip) would die at run-time with a overflowed number. die "Range iterator outside integer range". $count := -1 will lead to test timeouts. Note that cperl doesn't yet check UInt types at run-time for negative values. This might change in later versions with the use types pragma.

scalar(@array) for array or hash length is also bad code, it needs to be replaced with int(@array).
Such a length can never be a NV or PV, it is always a UInt. Using int() is clearer, better and faster.

Member

rurban commented Jun 8, 2016

That's true.
I'll make the args optional but keep the strict types.

This is now a FAQ:

So that when you use one or two args for Test::More::skip(), they need to properly typed.

I.e.

skip $why, 1` => `skip "$why", 1
skip "why", scalar(@skips) => skip "why", int(@skips)
skip "why", 2*$skips => skip "why", int(2*$skips)

Rationale:

skip() is a special case that the two args are very often mixed up. This error had previously to be detected at run-time with a fragile \d regex check. And in most cases this problem was never fixed, e.g. in Module::Build.
Only with checking for strict types I could detect and fix all of the wrong usages, get rid of the unneeded run-time check, the code is better, all errors are detected at compile time, and not covered at run-time and with the new strict types the code is much more readable, what is the Str $why and what the UInt $count argument.

$count can never be a NV, thus :Numeric (which would allow 2*$skip) is wrong. It needs to be :UInt. The used range op (pp_flip) would die at run-time with a overflowed number. die "Range iterator outside integer range". $count := -1 will lead to test timeouts. Note that cperl doesn't yet check UInt types at run-time for negative values. This might change in later versions with the use types pragma.

scalar(@array) for array or hash length is also bad code, it needs to be replaced with int(@array).
Such a length can never be a NV or PV, it is always a UInt. Using int() is clearer, better and faster.

@rurban rurban self-assigned this Jun 8, 2016

@rurban rurban added this to the v5.24.0 milestone Jun 8, 2016

@rurban rurban added the regression label Jun 8, 2016

rurban pushed a commit that referenced this issue Jun 8, 2016

Reini Urban
Test-Simple: relax skip types
Permit (str $why = "", Numeric $how_many = 0)
Update the FAQ in STATUS.
Improves #153. Stricter typechecks should be opt-in.
Currently we still dont have those pragmas yet.
no warning "types"; use types "cast"; use types "strict";
@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Jun 8, 2016

Member

Ok, I changed the $count from Int to Numeric, and allowed an empty skip with no_plan, as documented. The only backcompat problem is the str requirement, which will fail with plain Scalar(untyped) variables. If a variable, it needs to be stringified or typed. Otherwise I cannot detect the mixup at compile-time.

Member

rurban commented Jun 8, 2016

Ok, I changed the $count from Int to Numeric, and allowed an empty skip with no_plan, as documented. The only backcompat problem is the str requirement, which will fail with plain Scalar(untyped) variables. If a variable, it needs to be stringified or typed. Otherwise I cannot detect the mixup at compile-time.

@rurban rurban closed this in 4fa19ec Jun 8, 2016

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi
Contributor

ribasushi commented Jun 9, 2016

@ribasushi ribasushi reopened this Jun 9, 2016

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 9, 2016

Contributor

In particular this line still fails like this as of 397c9ace1f949

Contributor

ribasushi commented Jun 9, 2016

In particular this line still fails like this as of 397c9ace1f949

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Jun 9, 2016

Member

Sure. Package-Stash-XS-0.28 is broken.
You cannot load a XS module with this:
$^X -Ilib -e "require $_; print '$_ ok'"

--- ./t/00-compile.t~   2013-07-16 18:17:11.000000000 +0200
+++ ./t/00-compile.t    2016-06-09 11:36:14.000000000 +0200
@@ -58,7 +58,7 @@
     # fake home for cpan-testers
     # no fake requested ## local $ENV{HOME} = tempdir( CLEANUP => 1 );

-    like( qx{ $^X -Ilib -e "require $_; print '$_ ok'" }, qr/^\s*$_ ok/s, "$_ loaded ok" )
+    like( qx{ $^X -Mblib -Ilib -e "require $_; print '$_ ok'" }, qr/^\s*$_ ok/s, "$_ loaded ok" )
         for sort @modules;

     SKIP: {

Other than that, the type checker passes now the
skip "Test::Script needed to test script compilation", scalar(@scripts) if $@;
line.

Member

rurban commented Jun 9, 2016

Sure. Package-Stash-XS-0.28 is broken.
You cannot load a XS module with this:
$^X -Ilib -e "require $_; print '$_ ok'"

--- ./t/00-compile.t~   2013-07-16 18:17:11.000000000 +0200
+++ ./t/00-compile.t    2016-06-09 11:36:14.000000000 +0200
@@ -58,7 +58,7 @@
     # fake home for cpan-testers
     # no fake requested ## local $ENV{HOME} = tempdir( CLEANUP => 1 );

-    like( qx{ $^X -Ilib -e "require $_; print '$_ ok'" }, qr/^\s*$_ ok/s, "$_ loaded ok" )
+    like( qx{ $^X -Mblib -Ilib -e "require $_; print '$_ ok'" }, qr/^\s*$_ ok/s, "$_ loaded ok" )
         for sort @modules;

     SKIP: {

Other than that, the type checker passes now the
skip "Test::Script needed to test script compilation", scalar(@scripts) if $@;
line.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 9, 2016

Contributor

@rurban, please click the actual links I gave you in #153 (comment)

The logs clearly contradict

Other than that, the type checker passes now the skip "Test::Script needed to test script compilation", scalar(@scripts) if $@; line.

I very seldomly talk out of my ass, please take the time to look into my bugreports.

Contributor

ribasushi commented Jun 9, 2016

@rurban, please click the actual links I gave you in #153 (comment)

The logs clearly contradict

Other than that, the type checker passes now the skip "Test::Script needed to test script compilation", scalar(@scripts) if $@; line.

I very seldomly talk out of my ass, please take the time to look into my bugreports.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 9, 2016

Contributor

@rurban ... unless skip() needs to know $how_many tests are in the block at.. is a warning, in which case nothing to see here.

Contributor

ribasushi commented Jun 9, 2016

@rurban ... unless skip() needs to know $how_many tests are in the block at.. is a warning, in which case nothing to see here.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 9, 2016

Contributor

@rurban Yes, it is a warnings, sorry for the noise :/

Contributor

ribasushi commented Jun 9, 2016

@rurban Yes, it is a warnings, sorry for the noise :/

@ribasushi ribasushi closed this Jun 9, 2016

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Jun 9, 2016

Member

See rurban/distroprefs@e3535bd for the new 2 patches needed for Package::Stash::XS. This has nothing to do with cperl. Just sloppy tests.

Member

rurban commented Jun 9, 2016

See rurban/distroprefs@e3535bd for the new 2 patches needed for Package::Stash::XS. This has nothing to do with cperl. Just sloppy tests.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 9, 2016

Contributor

Yes, I agree. This time I did speak out of my ass ;)

Contributor

ribasushi commented Jun 9, 2016

Yes, I agree. This time I did speak out of my ass ;)

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi
Contributor

ribasushi commented Jun 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment