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.

• based on top of latest source code
• changelog entry included
• tests pass in CI
• git history is clean
• 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 👍 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.

[ferki]

Thanks for the patience about this PR!

I have to admit I had a hard time to wrap my head around all the details and implications. Last night I spent another few hours on it, and I could finally have some enlightenment about what's going on and how to go forward.

All in all, I like the new approach, and especially the new tests. Most comments are about:

• rounding up the story by checking related project history
• cleaning up perlcritic violations instead of disabling them
• simplifying the test approach

I believe most of the remaining actions here are more tedious than complex, and it might work well to even do it together.

I would suggest coordinating work some time via Matrix/IRC, or even pairing up under the umbrella of my open source office hours offering.

[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! 👍

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 (347e328). 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 👍

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 4a1ae27 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! 🙏), and also some of t/load_rexfile.t, this is my plan for the remaining todo items for reference:

• 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
  • add Rexfile loading tests
  • add tests for cleaning up loader strings in the output
• this is also a chance to apply a bit more cosmetic changes:
  • change Added to Add in the commit message
  • fix the commit author name from uralm1 to Ural Khasanov for consistency
• fix RegularExpressions class of critic violations:
  • add use re '/msx'; to the boilerplate, so we can have those on all regular expressions
  • fix the two cases of RegularExpressions::ProhibitUnusualDelimiters violations they were removed by testing output as a whole
• switch to match the logged content as a whole:
  • probably set logformat to disable the timestamp prefix of Rex::Logger output for simplified matching
  • 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)
• switch to testing the output instead on top of inspecting the log content:
  • convert the multiline like tests of the log's $content into a single output_like output_is call
  • clear up all the scaffolding around handling the logfiles
  • this should clear up all ProhibitPunctuationVars, RequireCheckedSyscalls, RequireCheckedClose, and Carping classes of critic violations
• check the remaining steps at this point:
  • 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
  • check alternatives for redefining Rex::CLI::exit in the tests (this probably also handles ProhibitNoWarnings and ProhibitPcakageVars classes of critic violations)
• 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]

This change ferki@03140ce 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 🤭).

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 🎉

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