-
Notifications
You must be signed in to change notification settings - Fork 223
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
Fix #1508 - the needs()
function should propagate calling task's params/args down to needed tasks
#1509
base: master
Are you sure you want to change the base?
Fix #1508 - the needs()
function should propagate calling task's params/args down to needed tasks
#1509
Changes from all commits
97f9e9a
93a5135
a011f27
4c33d32
cd465dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
|
||
=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 | ||
|
||
Comment on lines
+1
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rex tests currently do not include POD sections (maybe they should, though?! 🤔), but they aimed to be treated as standalone perl scripts with shebang, You can squash this change away into the original commit if you want, or I can change it upon merge as a follow-up commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @ferki, Thank you for your through review -- of this PR as well as the related issue. I will try to briefly address your remarks in several chunks. POD in test filesSince the For some kinds of content, I find it nicer than block/line comments. It renders better and folds neatly in the text editor. Whether or not it is socially allowed is up to the maintainer (you), so go ahead and yank it if you wish. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why were the tests marked as TODO ?The cherry-picking thing might have played a role in there, yes. If Meanwhile, here are my reasons, in a nutshell, in an attempt to convince you of the added value:
Basically, it tends to be a superior form of TDD, in my humble opinion:
And, of course, all of that can happen within the same PR (as was the case here). But you probably already know all this... And you probably have good reasons to prefer otherwise. In the end, whatever the maintainer of the project decides, of course. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. POD in testsIn general I'm not opposed to add POD to test files, provided we can realistically commit to establish it as the new norm, and also to start adding them retroactively for consistency. I feel that would involve a considerable overhead, so I'd wish to discuss that topic outside of the context of this PR (and focus on passing arguments to TODO testsHaving new tests that first pass, which then start to fail when I successfully implement their logic feels backward for my brain 🙃 I can accept it works better for you, and I appreciate your detailed reasoning ❤️ So far I used In the usual PR context, I like to have it demonstrated by the CI run that the new tests are actually failing before the related change gets implemented. So I push the "Add tests for X" commits early, and watch them fail ("red"). Then I also like to demonstrate that the new commit (or sometimes series of commits) I push to implement/fix the failing behavior actually makes the test suite to pass ("green"). Then if I need to simplify the code, I can make that at the end, and demonstrate the tests still pass ("refactor") If my PR would be about adding new tests that I don't intend to fix in the same PR, I think I'd still push a commit with the failing tests first, then make the CI pass with a second commit that marks them as TODO. I'd be happy to further discuss these methodology details via GitHub discussions or chat. |
||
use Test::More; | ||
use Rex::Commands; | ||
|
||
{ | ||
|
||
package T; # Helper package (for cutting down boilerplate in tests) | ||
use Storable; | ||
|
||
sub track_taskrun { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly this subroutine saves the arguments passed to it in a file via Storable. If that's true, would it be better to call it something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you wish so... It also saved the arguments, yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think by now I better understand the original name, thanks to your explanations. So this is a subroutine that tracks task execution (~run). |
||
my %opts = ref $_[-1] eq 'HASH' | ||
? %{ | ||
; | ||
pop | ||
} | ||
: (); | ||
Comment on lines
+28
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the calls in this file, it seems that the first argument is always a scalar value, and the last argument is always a hash reference. Does it make sense to still keep this unpack logic around? Maybe this works too: my ($called_task, %opts) = @_; That way we don't need the for loop either. What do you think? (Same goes for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps... The intention was the ability to do the following, when you have more than one needed task : needs ("task1", "task2", "task3")
check_needed("task1", "task2", "task3", \%opts) Currently, it looks like we don't have that case (with several needed tasks) in the tests. Maybe we should ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think that's what confused me initially. We don't have the multiple-tasks case tested now, so it might be premature optimizing. But your're right, in the long run we probably should cover that use case too 👍 |
||
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; | ||
ferki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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 /); | ||
}; | ||
|
||
{ | ||
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit I have a hard time to follow what exactly is happening in this test file. Could you walk me through it, please?
I see it was inspired by
t/needs.t
. Is there a simpler way to test the behavior perhaps? Or to fold the new checks intot/needs.t
directly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned, and despite how it may look at first, this test script was heavily inspired from
t/needs.t
.It's basically a refactored version of it, with one notable functional difference: unlike
t/needs.t
, this one also checks the run-time task options and params.So, my initial -joking- response would be :
sure, please walk us through
t/needs.t
and I will do the same for this one :-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Joking aside, here's a brief run-through :
Just like in
t/needs.t
, themain
package does the following :Just like in
t/needs.t
, some of those tasks invoke theneeds()
function, either with other tasks in themain
package, or tasks defined in other packages (Rex::Module
orNested::Module
) which also reside in the same test script.1) Why do we have the two other packages (
Rex::Module
andNested::Module
) ?Same reason as we have them in
t/needs.t
, i.e. we want to check that theneeds()
function works across package namespaces.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2) Why do we have an extra helper package (T), unlike in
t/needs.t
?Well, that's where the DRY refactoring happens...
Notice how almost the same code is repeated again and again in the task subs in
t/needs.t
.Here, that boilerplate is replaced with calls to
track_taskrun
andcheck_needed
which sensibly do the same, i.e. :track_taskrun
: When a task gets run, mark it as such by writing to a file with that name (and also save the its arguments there).check_needed
:die
unless a file with that name existsdie
unless the arguments that were saved in that file correspond to the expected argumentsDoing this here might not be considered very orthodox, but that's also what's happening in
t/needs.t
)Notice how failure is signaled with
die
, just like int/needs.t
.This also partly explains why we employ the private
_deep_check()
routine fromTest::More::
here.Just like you, my first reflex was employing
is_deeply
, but that doesn't work because "Test::More" chokes as it is then unable to keep track of the actual number of tests.The cleaner alternative would have been to rely on
is_deeply
in themain
test loopfor my $task ( $run_list->tasks )
in the test script's packagemain
, but that won't work either, because the summary info (from$task_list->get_summary
) does not contain task params/arguments... Well it probably should... :-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3) Why do we have to do file IO in the first place ?
I was also suprised by this at first. I guess the author of
t/needs.t
could have the best answer here.The explanation I came up with has to do with the task execution model in Rex (each task possibly having separate SSH connection, possibly running on a separate thread/process, ...).
A no-brainer method for eliminating the hairiness that would get involved is to go through file IO.
t/needs.t
does this quite simply by writing/truncating a file that has the same name of the needed task when its being run, and then merely checking the existence of the file from the calling task.Here, we also keep track of the params/arguments, so we use
Storable
to do that, which also eliviates the need for manually opening and reading/writing to files.I chose
Storable
because of its simplicity and popularity. Besides,Rex
already has a runtime dependency onStorable
, so using it in a test did not entail adding a dependency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point you ask if we could get rid of the unpacking logic in helper routines, which was actually neatly placed on a single line (before perl tidy messed it up :-) :
Yes, we could; by directly assuming a HASH reference as the last argument, like such :
But, if you know of a way of preventing perl-tidy doing its thing, I think it would be preferable to keep the unpacking logic in there.
It gives the caller more freedom. That way, if desired, the current tests in
t/needs.t
can easily be implemented with those two refactored routines (track_taskrun
andcheck_needed
).The above also gives away my thinking about you suggestion of merging these tests into
t/needs.t
.Imho, the easiest and most maintainable way of doing that would be retrofit
t/needs.t
tests into this -refactored- model.Otherwise, if we try to it the non-DRY way
t/needs.t
does this (by copying and pasting the file IO code in all tasks), we could easily end up with a mess of several hundred lines (if not more) of hard-to-maintain code...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, later I realized it is based on
t/needs.t
. My take is thatt/needs.t
is bad too, but that's part of the historical ballast from 10 years ago. That's a considerable amount of legacy to clean up, so that's why we use Test::Perl::Critic::Progressive to make sure we at least don't add more violations on top of the current pile :) These tests were unknowingly broken for PRs coming from forks, which was my mistake and I fixed that now (so it should properly complain aboutt/issue/1508.t
after a rebase).In other words, the policy is "we accept our legacy, but we don't want to add more ballast; therefore new code should be clean, or clean up old mess".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation!
Rex chops off the leading
Rex::
part for task names for brevity. We have these two modules defined int/needs.t
to make sureneeds
work with namespaces both with and without theRex::
prefix.That sounds like a good indicator to invent a cleaner approach than the 10-year-old legacy
t/needs.t
- or even better, fixt/needs.t
first :)I wonder if we could split
t/needs.t
up into real modules under e.gt/lib
first as a pure refactor, then modify it to support running the same test tasks both with and without arguments 🤔Yup, I see we use files to keep track of some execution data, and I don't have a much better way for that currently.
Storable
also sounds good for dumping/restoring the extra data needed by the argument tests. It is also a Perl core module, so it wouldn't be a new dependency anyway 👍That might also be an indicator of weirdness :) I'm happy to tune perltidy rules and reformat the codebase with that.
In this case though perlcritic complains about the null statement (and in the exploded formatting, about the missing final semicolon in the block). Perhaps it would be more readable as fully unpacking @_ first, and then do this transformation logic on the last element of the passed array?