Test-More / Test2-Harness

Alternative to Test::Harness
Other
23 stars 26 forks source link

Arguments after arisdottle (::) disappear from @ARGV #195

Closed oalders closed 4 years ago

oalders commented 4 years ago

This appears to be the behaviour since v1.000024. Specifically it looks to break at https://github.com/Test-More/Test2-Harness/commit/e23a37d5#diff-4a4cd0f6c984be5b67002a3b8c1b99b2R277

The line in question: push @{$task->{test_args}} => @{$res->{args}};

If I comment out that line, then arguments following the arisdottle re-appear.

Test case:

use strict;
use warnings;

use Test::More;

is (scalar @ARGV, 1);
is ( $ARGV[0], 'foobar');

done_testing();

Invoke via perl -Ilib scripts/yath test.t :: foobar

0$ git rev-parse HEAD
e23a37d5c00c1c2aa4da5c003751069731cdb113
$ perl -Ilib scripts/yath test.t :: foobar

** Defaulting to the 'test' command **

[  FAIL  ]  job  1  + <UNNAMED ASSERTION>
(  DIAG  )  job  1      Failed test at test.t line 6.
(  DIAG  )  job  1             got: '0'
(  DIAG  )  job  1        expected: '1'
[  FAIL  ]  job  1  + <UNNAMED ASSERTION>
(  DIAG  )  job  1      Failed test at test.t line 7.
(  DIAG  )  job  1             got: undef
(  DIAG  )  job  1        expected: 'foobar'
(  DIAG  )  job  1    Looks like you failed 2 tests of 2.
( FAILED )  job  1    test.t
< REASON >  job  1    Test script returned error (Err: 2)
< REASON >  job  1    Assertion failures were encountered (Count: 2)

The following jobs failed:
+--------------------------------------+-----------+
| Job ID                               | Test File |
+--------------------------------------+-----------+
| 6DFB56FC-FEA3-11EA-8D99-5873BE12D0D4 | test.t    |
+--------------------------------------+-----------+

                                Yath Result Summary
-----------------------------------------------------------------------------------
     Fail Count: 1
     File Count: 1
Assertion Count: 2
      Wall Time: 0.40 seconds
       CPU Time: 0.64 seconds (usr: 0.19s | sys: 0.04s | cusr: 0.30s | csys: 0.11s)
      CPU Usage: 159%
    -->  Result: FAILED  <--

$ git checkout HEAD~1
Previous HEAD position was e23a37d5 Add resource management classes
HEAD is now at 1d88a3b4 Merge pull request #190 from Test-More/haarg-patch-1
$ perl -Ilib scripts/yath test.t :: foobar

** Defaulting to the 'test' command **

( PASSED )  job  1    test.t

                                Yath Result Summary
-----------------------------------------------------------------------------------
     File Count: 1
Assertion Count: 2
      Wall Time: 0.44 seconds
       CPU Time: 0.63 seconds (usr: 0.18s | sys: 0.04s | cusr: 0.30s | csys: 0.11s)
      CPU Usage: 142%
    -->  Result: PASSED  <--
exodist commented 4 years ago

This is probably a bug in the runner logic that handles the situation where multiple conflicting settings get picked. For instance you can set a command line value for timeout, and tests can also set test-specific timeouts. Typically it is done as a fallback, it picks one. This push vivifies args, even when none are present, so the ones coming from the :: never have a chance to come in.

I think CLI args is a special case where things should be merged, not fallback. The question is ordering, I think arguments can be made in either direction. I think :: args should come last, Other sources are generally test-specific, and as such for any given test will be consistent (from plugins, directives, resources, etc). The :: cli ones are the ones that may come and go or be inconsistent.

oalders commented 4 years ago

Right. In our codebase we're using Test::Class::Moose and passing in test class names after the ::. We've fallen back to prove in the meantime because yath will no longer allow us to run individual test classes.

exodist commented 4 years ago

yeah, I will fix this tonight if I have a moment. Test::Class is used where I work, so I need to fix it for us too.

exodist commented 4 years ago

Released v1.000028

oalders commented 4 years ago

Thanks, @exodist! 🚀 Do you want me to send a PR with a regression test?

exodist commented 4 years ago

Sure!