From 97f9e9a2ae3b4e54c5177769525f25a0a5d9ee74 Mon Sep 17 00:00:00 2001 From: Tabulo Date: Mon, 27 Sep 2021 16:08:51 +0200 Subject: [PATCH 1/5] Add "TODO" tests related to issue #1508 The added tests check that the needs() function propagates run-time task arguments (%params and @args) from the calling task down to the "needed" tasks. CHANGES: ============= new file: t/issue/1508.t HOW TO TEST : ============= $ prove -v t/issue/1508.t # for this issue $ prove t/**/*.t # for non-regression While the related tests remain marked as "TODO", they will not report failures during normal test runs. To see their true pass/fail status, you have to pass the '-v' option to `prove`. --- t/issue/1508.t | 167 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 t/issue/1508.t diff --git a/t/issue/1508.t b/t/issue/1508.t new file mode 100644 index 000000000..a1ec556b4 --- /dev/null +++ b/t/issue/1508.t @@ -0,0 +1,167 @@ + +=head1 NAME + +issue/1508.t - Check that the `needs()` function correctly propogates run-time task arguments + +=head1 DESCRIPTION + +Check that the `needs()` function does indeed correctly propogate +run-time task arguments (%params and @args) from the calling task down to the "needed" tasks. + +=head1 DETAILS + + * AUTHOR / DATE : [tabulon]@[2021-09-26] + * RELATES-TO : [github issue #1508](https://github.com/RexOps/Rex/issues/1508#issue-1007457392) + * INSPIRED from : t/needs.t + +=cut + +use Test::More; +use Rex::Commands; + +{ + + package T; # Helper package (for cutting down boilerplate in tests) + use Storable; + + sub track_taskrun { + my %opts = ref $_[-1] eq 'HASH' + ? %{ + ; + pop + } + : (); + my $argv = $opts{argv}; + my @prereqs = (@_); + for (@prereqs) { + my $file = "${_}.txt"; + Storable::store( $argv, $file ); + } + } + + sub check_needed { + my %opts = ref $_[-1] eq 'HASH' + ? %{ + ; + pop + } + : (); + my $argv = $opts{argv}; + my $do_unlink = delete $opts{unlink} // 1; + my @prereqs = (@_); + + for (@prereqs) { + my $file = "${_}.txt"; + + -f "$file" or die; + my $propagated_argv = Storable::retrieve("$file"); + Test::More::_deep_check( $propagated_argv, $argv ) or die; + + unlink("$file") if ($do_unlink); + } + } +} + +{ + + package MyTest; + use strict; + use warnings; + use Rex::Commands; + + $::QUIET = 1; + + task test1 => sub { + T::track_taskrun( test1 => { argv => \@_ } ); + }; + + task test2 => sub { + T::track_taskrun( test2 => { argv => \@_ } ); + }; + + 1; +} + +{ + + package Nested::Module; + + use strict; + use warnings; + + use Rex::Commands; + + task test => sub { + T::track_taskrun( test => { argv => \@_ } ); + }; +} + +{ + + package Rex::Module; + + use strict; + use warnings; + + use Rex::Commands; + + task test => sub { + T::track_taskrun( test => { argv => \@_ } ); + }; +} + +task test => sub { + needs MyTest; + + T::check_needed( $_, { argv => \@_ } ) for (qw/test1 test2/); +}; + +task test2 => sub { + needs MyTest "test2"; + + T::check_needed( $_, { argv => \@_ } ) for (qw/test2/); +}; + +task test3 => sub { + needs "test4"; + + T::check_needed( $_, { argv => \@_ } ) for (qw/test4/); +}; + +task test4 => sub { + T::track_taskrun( test4 => { argv => \@_ } ); +}; + +task test5 => sub { + needs Nested::Module test; + + T::check_needed( $_, { argv => \@_ } ) for (qw/test/); +}; + +task test6 => sub { + needs Rex::Module "test"; + + T::check_needed( $_, { argv => \@_ } ) for (qw/test /); +}; + +TODO: { + local $TODO = "Issue 1508: The needs() function should propogate parameters/args"; + + my $task_list = Rex::TaskList->create; + my $run_list = Rex::RunList->instance; + $run_list->parse_opts(qw/test test2 test3 test5 test6/); + + for my $task ( $run_list->tasks ) { + my $name = $task->name; + my %prms = ( "${name}_greet" => "Hello ${name}" ); + my @args = ( "${name}.arg.0", "${name}.arg.1", "${name}.arg.2" ); + + $task_list->run($task); + + my @summary = $task_list->get_summary; + is_deeply $summary[-1]->{exit_code}, 0, $task->name; + $run_list->increment_current_index; + } +} + +done_testing; From 93a513533e0e2bd56f3853b4080a86b07b88fa06 Mon Sep 17 00:00:00 2001 From: Tabulo Date: Mon, 27 Sep 2021 16:12:22 +0200 Subject: [PATCH 2/5] Fix #1508 simply by correcting typo on a couple lines Notice that how params and args were mixed up (interchanged) when being passed down -- within the needs() function. This appears to be a typo, introduced initially by PR #1157 (the fix for #1066 ) with 48c737b. CHANGES: ============= modified: lib/Rex/Command.pm HOW TO TEST : ============= $ prove -v t/issue/1508.t # for this issue $ prove t/**/*.t # for non-regression While the related tests remain marked as "TODO", they will not report failures during normal test runs. To see their true pass/fail status, you have to pass the '-v' option to `prove`. --- lib/Rex/Commands.pm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Rex/Commands.pm b/lib/Rex/Commands.pm index 2280172dc..fc0e03a81 100644 --- a/lib/Rex/Commands.pm +++ b/lib/Rex/Commands.pm @@ -1066,11 +1066,11 @@ sub needs { my $suffix = $self . ":"; if ( @args && grep ( /^\Q$task_name\E$/, @args ) ) { Rex::Logger::debug( "Calling " . $task_o->name ); - $task_o->run( "", params => \@task_args, args => \%task_opts ); + $task_o->run( "", params => \%task_opts, args => \@task_args ); } elsif ( !@args ) { Rex::Logger::debug( "Calling " . $task_o->name ); - $task_o->run( "", params => \@task_args, args => \%task_opts ); + $task_o->run( "", params => \%task_opts, args => \@task_args ); } } From a011f27f543e4b86639f425565d45995cbf5e4db Mon Sep 17 00:00:00 2001 From: Tabulo Date: Mon, 27 Sep 2021 16:39:02 +0200 Subject: [PATCH 3/5] Fix #1508 by correcting a typo + minimal code cleanup This is an alternative fix which also does some minimal code clean-up as well fixing the culprit (typo). It is proposed in a separate commit to ease cherry-picking. In any case, all tests pass either way. JUSTIFICATION : =============== There does not appear to be any particular reason for having two identical invocations of the ->run method in two separate arms of an `if... elsif...` statement. So this commit replaces them by a -logically equivalent- snippet. FURTHER DETAILS: ================= BTW, digging in repo history, it seems that the two arm `if ... elsif ...` form existed since the very initial introduction of the needs() function by commit 95d3e91. But even at that time (and probably ever since), those two arms of the if statement always did exactly the same thing... So I can't think of any valid reason to keep them around. CHANGES: ============= modified: lib/Rex/Command.pm HOW TO TEST : ============= $ prove -v t/issue/1508.t # for this issue $ prove t/**/*.t # for non-regression --- lib/Rex/Commands.pm | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/Rex/Commands.pm b/lib/Rex/Commands.pm index fc0e03a81..250d127c1 100644 --- a/lib/Rex/Commands.pm +++ b/lib/Rex/Commands.pm @@ -1064,11 +1064,7 @@ sub needs { my $task_o = $tl->get_task($task); my $task_name = $task_o->name; my $suffix = $self . ":"; - if ( @args && grep ( /^\Q$task_name\E$/, @args ) ) { - Rex::Logger::debug( "Calling " . $task_o->name ); - $task_o->run( "", params => \%task_opts, args => \@task_args ); - } - elsif ( !@args ) { + if ( !@args || grep ( /^\Q$task_name\E$/, @args ) ) { Rex::Logger::debug( "Calling " . $task_o->name ); $task_o->run( "", params => \%task_opts, args => \@task_args ); } From 4c33d3204a7d9c4223a765e3833e42b88d01f28a Mon Sep 17 00:00:00 2001 From: Tabulo Date: Mon, 27 Sep 2021 16:40:39 +0200 Subject: [PATCH 4/5] Update ChangeLog, remove "TODO" mark from tests related to #1508 This is made in a separate commit so to ease cherry-picking between two alternative fixes proposed in distinct commits. In any case, all tests pass either way. CHANGES: ============= modified: t/issue/1508.t modified: ChangeLog HOW TO TEST : ============= $ prove -v t/issue/1508.t # for this issue $ prove t/**/*.t # for non-regression --- ChangeLog | 1 + t/issue/1508.t | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2fa7c11b8..df6d70e97 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,6 +5,7 @@ Revision history for Rex [BUG FIXES] - Detect invalid hostgroup expressions + - Propagate args/params down to "needed" tasks launched by needs() [DOCUMENTATION] diff --git a/t/issue/1508.t b/t/issue/1508.t index a1ec556b4..794e59667 100644 --- a/t/issue/1508.t +++ b/t/issue/1508.t @@ -144,9 +144,9 @@ task test6 => sub { T::check_needed( $_, { argv => \@_ } ) for (qw/test /); }; -TODO: { - local $TODO = "Issue 1508: The needs() function should propogate parameters/args"; + +{ my $task_list = Rex::TaskList->create; my $run_list = Rex::RunList->instance; $run_list->parse_opts(qw/test test2 test3 test5 test6/); From cd465dcad34912c597b6540b3bc7b3b04106ec9c Mon Sep 17 00:00:00 2001 From: Tabulo Date: Mon, 27 Sep 2021 19:12:37 +0200 Subject: [PATCH 5/5] Tidy tests related to #1508 (to satisfy xt/author/perltidy.t) `dzil test -all` was failing on `xt/author/perltidy.t`, apparently not happy with the indentation/tab-stops. CHANGES: ============= modified: t/issue/1508.t HOW TO TEST : ============= ```shell $ prove --lib -v t/issue/1508.t # for this issue $ prove --lib --recursive t/ # for non-regression ``` --- t/issue/1508.t | 2 -- 1 file changed, 2 deletions(-) diff --git a/t/issue/1508.t b/t/issue/1508.t index 794e59667..973c7ca30 100644 --- a/t/issue/1508.t +++ b/t/issue/1508.t @@ -144,8 +144,6 @@ task test6 => sub { T::check_needed( $_, { argv => \@_ } ) for (qw/test /); }; - - { my $task_list = Rex::TaskList->create; my $run_list = Rex::RunList->instance;