Fix #1521 lib/Rex/CLI.pm: Use $SIG{__WARN__} when loading Rexfile instead of stderr redirection

[uralm1]

This PR is an attempt to fix #1521 Also test for load_rexfile() is added.

• [x] based on top of latest source code
• [x] changelog entry included
• [x] tests pass in CI
• [x] git history is clean
• [x] git commit messages are well-written

[uralm1]

It seems brutal Perl::Critic warnings must be approved too :)

# CodeLayout::ProhibitParensWithBuiltins: Got 1237 violation(s).  Expected no more than 1236.
2022-02-04T05:54:46.6712235Z # CodeLayout::RequireTrailingCommaAtNewline: Got 449 violation(s).  Expected no more than 443.
2022-02-04T05:54:46.9932056Z # Community::WhileDiamondDefaultAssignment: Got 4 violation(s).  Expected no more than 3.
2022-02-04T05:54:47.0420687Z # Compatibility::PerlMinimumVersionAndWhy: Got 151 violation(s).  Expected no more than 144.
2022-02-04T05:54:47.1343437Z # ControlStructures::ProhibitPostfixControls: Got 317 violation(s).  Expected no more than 315.
2022-02-04T05:54:47.4025032Z # ErrorHandling::RequireCarping: Got 477 violation(s).  Expected no more than 474.
2022-02-04T05:54:47.5341814Z # InputOutput::RequireBracedFileHandleWithPrint: Got 80 violation(s).  Expected no more than 79.
2022-02-04T05:54:47.5548372Z # InputOutput::RequireCheckedClose: Got 47 violation(s).  Expected no more than 43.
2022-02-04T05:54:47.5783578Z # InputOutput::RequireCheckedSyscalls: Got 240 violation(s).  Expected no more than 238.
2022-02-04T05:54:47.7863122Z # Modules::RequireEndWithOne: Got 71 violation(s).  Expected no more than 70.
2022-02-04T05:54:47.7975985Z # Modules::RequireExplicitPackage: Got 69 violation(s).  Expected no more than 68.
2022-02-04T05:54:47.8296391Z # Modules::RequireVersionVar: Got 83 violation(s).  Expected no more than 82.
2022-02-04T05:54:47.9288448Z # RegularExpressions::ProhibitFixedStringMatches: Got 40 violation(s).  Expected no more than 27.
2022-02-04T05:54:47.9577862Z # RegularExpressions::ProhibitUnusualDelimiters: Got 10 violation(s).  Expected no more than 7.
2022-02-04T05:54:47.9895990Z # RegularExpressions::RequireDotMatchAnything: Got 932 violation(s).  Expected no more than 907.
2022-02-04T05:54:48.0008322Z # RegularExpressions::RequireExtendedFormatting: Got 891 violation(s).  Expected no more than 868.
2022-02-04T05:54:48.0128342Z # RegularExpressions::RequireLineBoundaryMatching: Got 914 violation(s).  Expected no more than 889.
2022-02-04T05:54:48.1593293Z # Subroutines::RequireFinalReturn: Got 649 violation(s).  Expected no more than 648.
2022-02-04T05:54:48.1818982Z # TestingAndDebugging::ProhibitNoWarnings: Got 15 violation(s).  Expected no more than 14.
2022-02-04T05:54:48.2360229Z # TooMuchCode::ProhibitDuplicateLiteral: Got 3466 violation(s).  Expected no more than 3463.
2022-02-04T05:54:48.4556396Z # ValuesAndExpressions::ProhibitEmptyQuotes: Got 299 violation(s).  Expected no more than 297.
2022-02-04T05:54:48.8196941Z # Variables::ProhibitPackageVars: Got 68 violation(s).  Expected no more than 67.
2022-02-04T05:54:48.8390315Z # Variables::ProhibitPunctuationVars: Got 496 violation(s).  Expected no more than 493.
2022-02-04T05:54:48.8586466Z # Variables::ProhibitUnusedVariables: Got 18 violation(s).  Expected no more than 17.
2022-02-04T05:54:48.9210730Z # Too many Perl::Critic violations...
2022-02-04T05:54:48.9211161Z # Got a total of 20622. Expected no more than 20498.
2022-02-04T05:54:48.9224683Z 
2022-02-04T05:54:48.9225384Z #   Failed test 'Test::Perl::Critic::Progressive'
2022-02-04T05:54:48.9292323Z ##[error]#   at xt/author/critic-progressive.t line 24.
2022-02-04T05:54:48.9642559Z # Looks like you failed 1 test of 1.
2022-02-04T05:54:48.9957663Z [05:54:48] xt/author/critic-progressive.t .. 
2022-02-04T05:54:48.9958040Z Dubious, test returned 1 (wstat 256, 0x100)
2022-02-04T05:54:48.9958294Z Failed 1/1 subtests 
2022-02-04T05:54:48.9959621Z [05:54:48]

[ferki]

Thanks, @uralm1, for the pull request, updating it, and for your patience. I see it now passes all the tests, so I can safely start reviewing it to understand the proposed changes.

I am particularly excited about the Rexfile loading tests :+1: That idea came up recently related to other topics as well.

It seems brutal Perl::Critic warnings must be approved too :)

Well, usually not "approved", but fixed properly or cleaned up ;) Either by fixing the violation itself (preferred), configuring the rule (also good), or deciding that a specific rule is not important at all for the codebase as a whole, and then ignore it (sometimes it's the right thing to do).

Rex codebase is a result of 100+ contributors over more than a decade, so the code quality is a big mix of everyone's preferences at the time they contributed. Therefore Test::Perl::Critic::Progressive is being used in the test suite to at least stop introducing further Perl::Critic violations. In other words, the list of violations should get shorter with each pull request, or at least should stay the same. There are also a few words about this in the Contributing guide under Code quality.

[uralm1]

Ok, @ferki, thank you for participation! I'll reorganize PR as suggested when I get free time (open source office hours - what a brilliant idea :) I sadly have to state that currently switched to ansible for new tasks due it windows support. I like perl and Rex approach (contrary to jinja and yaml programming), so I'm going to keep an eye on it.

[ferki]

I'll reorganize PR as suggested when I get free time

Thanks! If you don't mind working in a shared branch, I might be able to push most of the proposed changes here as well to keep it in a single place. I might also do a rebase instead of merging back the current default branch here to keep the history cleaner.

(open source office hours - what a brilliant idea :)

It might be rare in the general open source development world (mostly isolated individuals working async), but I find it's often best to collaborate in pairs or groups – so I offer the possibility to do so :) I'm glad you like the idea!

I sadly have to state that currently switched to ansible for new tasks due it windows support.

As long as it's the one that helps you most to get your stuff done, that's fine! :+1:

Rex can run on Windows, have limited capability to manage it locally, and there's some proof of concept code to remotely manage it too. I'm not aware of anyone having proprietary solutions built in-house for full Windows support, though. It also seems nobody stepped up yet to develop, maintain, or sponsor the same as an open solution. If you are interested, let's chat!

I like perl and Rex approach (contrary to jinja and yaml programming), so I'm going to keep an eye on it.

Right, the approach to coding is a major difference. While I love the freedom granted by a mature and proper programming language, I also like the diversity of projects so everyone may find the one that fits their use case the most.

[ferki]

FYI, I hacked on this for a while, and by tidying up the split out sample Rexfiles, I managed to find a further bug: _reset_test does not clean up all the state between the test cases. Changing all the sample task names to test for consistency brought this out.

I decided to just add Rex::TaskList->create->clear_tasks(); to _reset_test as a hotfix, but it would be nice if we could further isolate the individual test cases. I'm considering to switch to a declarative approach by describing test inputs with their expected outputs in a hash, and then iterating through them in a loop. I expect that would make it possible to let all the state out of scope between test runs, this destroying the underlying objects - we'll see about the details later.

In any case, I started to push my related work into my personal fork under the same branch name.

[ferki]

Oops, didn't mean to push here yet, so I restored this to the previous HEAD (347e328d57ff5bd07f19ba893b5a50ac70ca1eca). I might push here when I'm ready, sorry for the noise.

[ferki]

Thanks @uralm1 for the updates! Splitting those commits out are useful, and I'm going to update my fork's branch with them :+1:

I think I figured out most of the remaining steps already in my local experiments, but I still need to split them into logical step-by-step commits, so I haven't pushed the full story yet. I just note this so we don't accidentally end up duplicating work.

[ferki]

OK, so I pushed an updated version to my branch, so you can see the outline of changes I've already made. It's basically:

• add the tests first ("tests first" – so we can work toward a specified expected behavior instead of describing the behavior after the changes we made)
• declare test plan
• fix test boilerplate (since your recent update it became cosmetic only)
• splitting out individual sample Rexfiles (fixes two classes of critic violations)
• make them more consistent
• fix the bug exposed by using the same task names in all sample Rexfiles
• revert the remaining parts of 4a1ae275d185ab7186fa5cd7b5f807aa28be64e7 that has not been reworked yet, but became unnecessary due to not having to do STDERR gymnastics anymore

Since you cleaned up Rex::CLI already (thanks! :pray:), and also some of t/load_rexfile.t, this is my plan for the remaining todo items for reference:

• [x] ~~maybe split tests into two commits:~~ since I went with testing the expectations about the output as a whole, there was no need to split the initial commit
  • [x] ~~add Rexfile loading tests~~
  • [x] ~~add tests for cleaning up loader strings in the output~~
• [x] this is also a chance to apply a bit more cosmetic changes:
  • [x] change Added to Add in the commit message
  • [x] fix the commit author name from uralm1 to Ural Khasanov for consistency
• [x] fix RegularExpressions class of critic violations:
  • [x] add use re '/msx'; to the boilerplate, so we can have those on all regular expressions
  • [x] ~~fix the two cases of RegularExpressions::ProhibitUnusualDelimiters violations~~ they were removed by testing output as a whole
• [x] switch to match the logged content as a whole:
  • [x] probably set logformat to disable the timestamp prefix of Rex::Logger output for simplified matching
  • [x] convert individual like tests into matching ~~(most of?)~~ the output as a whole ~~(I expect the /x modifier to be real handy here, because each bit of the expected matches can be described as comments)~~
• [x] switch to testing the output ~~instead~~ on top of inspecting the log content:
  • [x] convert the multiline like tests of the log's $content into a single ~~output_like~~ output_is call
  • [x] clear up all the scaffolding around handling the logfiles
  • [x] this should clear up all ProhibitPunctuationVars, RequireCheckedSyscalls, RequireCheckedClose, and Carping classes of critic violations
• [x] check the remaining steps at this point:
  • [x] ~~I'm considering to configure the CodeLayout::RequireTrailingCommaAtNewline critic policy with except_function_calls = 1 in .perlcriticrc separately on the current default branch – this should be closer to what we need in practice, and then we may simply rebase~~ after the cleanup it was not needed here, so it may happen on its own separate from this PR
  • [x] check alternatives for redefining Rex::CLI::exit in the tests (this probably also handles ProhibitNoWarnings and ProhibitPcakageVars classes of critic violations)
• [x] probably converting the test structure to be more declarative to loop over a list of test cases instead of executing them serially

[uralm1]

Tested https://github.com/ferki/Rex/tree/load_rexfile_test_and_fix - all is good, but I have concerns about strict comparsion of logs. This can make test fragile... to perl version or (may be) windows build.

[uralm1]

load_rexfile.t (variant with sample log files) under windows10 strawberry perl5.32.1 - works.

[ferki]

Tested https://github.com/ferki/Rex/tree/load_rexfile_test_and_fix - all is good

Thanks for the feedback!

but I have concerns about strict comparsion of logs. This can make test fragile...

The regex-based version I worked on started to become more complex than I preferred, so that's why I started to experiment on checking exact output. These approaches have different trade-offs for sure, that's why I'm exploring them.

About fragility:

• on fatal loading errors, the path to Rex::CLI is part of the message, which is different on each system that runs the test => I went around that with substituting the %REX_CLI_PATH% placeholder in the expected output to the given system's path to the module.

• on fatal loading errors, the line where it fails in Rex::CLI is also printed, which would change every time we move it in Rex::CLI => I don't like that fully, but in my experience this part of Rex is not being changed a lot, so I might accept this risk to maintain, and solve it later when it becomes a problem

Overall, I agree there's risk in there, but based on my local experiments of various approaches, this seems to be acceptable to me so far.

I wonder how useful it is to include the Rex::CLI path and line number in the error report, though :thinking: It might still be good to hide it in the failure message (because the user-fixable error is in the user's Rexfile and modules), and push this bit of into into a debug message instead (so the Rex internal locations related to the failure may still be discoverable).

to perl version or (may be) windows build.

GitHub Actions are testing with latest Perl version on Ubuntu, Mac and Windows. So far the tests pass on all of these combinations, and that's promising.

I consider adding a BSD flavor, as well as multiple Perl versions to test on (via separate issue).

Later CPAN Testers would also smoke a prerelease on various operating systems and Perl versions.

[ferki]

fix the commit author name from uralm1 to Ural Khasanov for consistency change Added to Add in the commit message

git itself makes it possible to rewrite any part of commits by others, so technically I am able to apply those cosmetic changes. Though as a principle, I prefer to to preserve contributors' commits in their original form. Merging/rebasing on top of current default branch, and perhaps splitting/squashing may be the only exceptions.

By "cosmetic change", I mean it might be OK to leave it as it is, but changing them would lead an increased consistency of:

• commit message style (~use imperative mood)
• autogenerated CONTRIBUTORS file (the alternative is to add an entry to .mailmap to indicate the two different authors are in fact one)

Added load_rexfile() test (1bea0ce229fe9d78e485c83fc898e4c029b0ecaa) is the one which has a different style and author than the rest.

@uralm1: Would you like me to apply those cosmetic changes to the single outlier commit?

[uralm1]

Yes, of cause, thanks.

[ferki]

I'm ready with all the things I planned or noticed along the way. I have to admit it proved to be more involved than I expected, but I feel satisfied with how it ended up. I also feel it was worth the effort to dig deep into this relatively slow changing, but central part of the code base, and the new approach feels much cleaner than before (and has tests now! :tada: ).

My changes are in my fork's branch now, but I may push here soon-ish too, and then I'll go through all the comments from above again to double-check everything after giving it a little rest to settle. I will also look for potential regressions, though the currently tested expectations feel good to me.

@uralm1: if you can do a quick sanity check on your systems, it would be nice to know about the results!

[uralm1]

Cool, that critic monster was defeated Ж:) !

• test passed successfully.
• redefine warning still here. I've understood you kept it intentionaly.

Should critiсize the approach to generate $expected hash in _setup_test and testing in a loop. It hides test logic in algorithmic constructs, making it hard to understand. On the other side, approach - create file.thing when we need to check thing is simple enough. Hmm.

Will test more after the weekends!

[ferki]

Cool, that critic monster was defeated Ж:) !

\o/ :tada:

Even better, by going through understanding each of the initial violations here, I could identify a few cases where we can configure perlcritic to better align with expectations. That's a nice extra benefit on the side, and I'll make those changes separately.

• test passed successfully.
• redefine warning still here. I've understood you kept it intentionaly.

Thanks for the feedback!

I am quite baffled by the redefine warning you seem to keep getting. It doesn't show up on my development machine, my separate personal CI, nor on GitHub actions.

As far as I can tell, there's no Rex::CLI::exit() function that gets redefined. Rex::Commands::exit() was mentioned in code comment, but that is also not imported by Rex::CLI, so doesn't get redefined.

Do you have any further tips or details on how to reproduce your case better?

Should critiсize the approach to generate $expected hash in _setup_test and testing in a loop. It hides test logic in algorithmic constructs, making it hard to understand. On the other side, approach - create file.thing when we need to check thing is simple enough. Hmm.

Once I chose with the proposed data-driven test approach after evaluating a few other ways, all the test cases ended up repeating the same steps. At that point, I chose to make it obvious and shortened the test code by forming a loop.

Will test more after the weekends!

Sounds great, thanks! I am also already running the updated code locally to give it more use in practice as well.

I consider pushing my branch here after I'm done with a few final cleanups, and if all's well, I'm going to merge it too.

[ferki]

I am quite baffled by the redefine warning you seem to keep getting. It doesn't show up on my development machine, my separate personal CI, nor on GitHub actions.

As far as I can tell, there's no Rex::CLI::exit() function that gets redefined. Rex::Commands::exit() was mentioned in code comment, but that is also not imported by Rex::CLI, so doesn't get redefined.

Ah, now I see it in GitHub Actions in the annotations section of the build results (and the tests pass). I'll look into this, but I really wonder why it only happens on Windows? :thinking:

[uralm1]

Not windows, I run under ubuntu 22. It can be my fault too - not actual CLI.pm inside my system perl installation . I cannot check it right now.

[ferki]

Not windows, I run under ubuntu 22. It can be my fault too - not actual CLI.pm inside my system perl installation . I cannot check it right now.

Interesting, I still could not fully reproduce it locally. I'm on Gentoo, using Perlbrew, and system perl has Rex installed as well.

In GitHub Actions, it only warned about this on Windows :shrug: Running the test suite on Linux and Mac did not result in a warning.

I might take another look later, since we need mocking in the test suite in other places already. I played with Sub::Override initially, but it didn't work for me (yet?).

I don't feel this PR is the good place to dig deeper into introducing mocking, though, so for now I decided to accept the ProhibitNoWarnings violation for that single line.

[uralm1]

This change https://github.com/ferki/Rex/commit/03140ce321f03a8699c34b4b883defacf863af61 hides? redirection warning. exit 1 @ CLI.pm 795 really calls Rex::Commands::exit. I can see it by 'INFO - Exiting Rex...' 'INFO - Cleaning up...' messages. This is from my old test with disabled redirection:

...
ok 9 - loader prefix is filtered in warnings report
[2023-01-21 00:41:16] ERROR - Compile time errors:
[2023-01-21 00:41:16] ERROR - 	syntax error at __Rexfile__.pm line 3, near "aaaabbbbcccc
[2023-01-21 00:41:16] ERROR - 	task "
[2023-01-21 00:41:16] ERROR - 	Compilation failed in require at /home/sv/src/Rex/lib/Rex/CLI.pm line 756.
<eval block in load_rexfile() failed and exit 1 is called>
<next messages comes from Rex::Commands::exit>
[2023-01-21 00:41:16] INFO - Exiting Rex...
[2023-01-21 00:41:16] INFO - Cleaning up...
<here CORE::exit stopped test>
# Tests were run but no plan was declared and done_testing() was not seen.

Definitely this needs more hacking.

[ferki]

No subroutine redefinition warnings on 5.36.0

According to the Changes to Existing Diagnostics section of perldelta for perl-5.36.0:

Localized subroutine redefinitions no longer trigger this warning.

My machines have 5.36.0, and GitHub Actions are using the latest perl version of, which also resolves to 5.36.0 - except on Windows, where we use Strawberry Perl, which resolves to 5.32.1 currently.

This explains why I did not get any warnings when I removed no warnings 'redefine'; from the code, but it still showed up in the GitHub Actions tests on Windows.

Overriding subroutines in the test suite

The above means we have a nice way of overriding subroutines without any perlcritic violations and without using external modules if we are running on 5.36.0 – but we do want to support older perl versions as well, so I took another look at the topic.

My second attempt at using Sub::Override for the replacing subroutines in the tests has also been successful, so I've updated my fork's branch with that. That module would be useful in other tests as well (like t/issue/948.t), and it seems widely available enough that I don't expect it to cause problems for downstream package maintainers who wish to also run tests (including myself :hand_over_mouth:).

Exit behavior

At this point I would like to draw a line somewhere around near here to stop inflating this PR with more and more concerns. I agree it might be useful to look into the exit behavior at various points in Rex core. If that is desired, let's do that separately unless it blocks the progress we already made here.

In my current understanding of the scope of this PR, overriding either Rex::CLI::exit or Rex::Commands::exit in t/load_rexfile.t would be acceptable to make the tests of the improved behavior pass. Testable Rexfile loading without STDERR hijacking magic, and with several identified points of further improvement is already a nice step forward compared to the current state of the default branch.

So I would propose to go towards merging the progress we already achieved, and do subsequent changes in separate smaller chunks on their own if possible.

[ferki]

Currently the minimum perl version needed by Rex is 5.10.1, but use re '/flags' is only supported since 5.14.0.

Since all the original regular expressions are gone due to the data-driven testing approach, I could also drop the two commits that were solving the RegularExpressions critic violations (and add flags manually in the remaining 2 places).

I plan to split out at least one other commit from here, as the one tuning the .perlcriticrc rules belongs to more of a general maintenance topic, and is useful project-wide as well.

I don't see any other obvious simplification opportunity, so I'll push my branch here after that to update the PR.

[ferki]

I've applied some new .perlcriticrc rules to the default branch, and rebased this PR on top of that. I've also decided to squash some of my commits together (where an extra commit didn't feel to add a useful intermediate step anymore).

Let's see the test results first, but now I consider it close to be merged.

[uralm1]

Looks good for me, can be tested on huma... be merged.

[ferki]

It is now merged :tada:

Thank you @uralm1 for your patience, and for your original work on both improving the test coverage and the general approach of Rexfile loading! :heart: