Re: [Bug-wget] [PATCH] Stylistic and idiomatic cleanups in Perl tests

[Pär Karlsson]

This particular piece of code is only executed if a test *fails, *in
_verify_download(). So as long as the tests succeded, this particular code
path was never executed. :-)

Best regards,

/Pär

2015-05-20 9:59 GMT+02:00 Tim Ruehsen <address@hidden>:

> Thank you, Pär and Hubert.
>
> A very good catch !
>
> Since none of our Perl tests failed though we have broken Perl code, the
> question arises, if this is dead code (not executed when running our
> tests) !?
>
> BTW, I would like to mention that we plan to translate the Perl tests into
> Python. In the end, we want to get rid of the Perl test suite.
> So, before you start spending your precious developer time into improving
> the
> Perl test suite, please consider improving the Python test suite (and/or
> move
> tests from Perl to Python).
>
> Anyways, i'll push your patch soon if nobody complains.
>
> Regards, Tim
>
> On Tuesday 19 May 2015 22:27:24 Pär Karlsson wrote:
> > Yes, you are right. The declaration of $i inside the for expression
> shadows
> > the scope of the outer $i inside the for loop (which indeed makes the
> outer
> > $i uninitialized after the loop).
> >
> > The for loop itself is not stylistically Perlish with the C-style loop,
> but
> > that's a minor gripe. I made an error when using the Perl-style range
> > operator ( N1 .. N2 ) loop and forgot to remove the commented out line
> too.
> > And I mistakenly left the extraneous 'my' there, which declares $i in a
> new
> > scope. After some unintentional off-list conversation with Hubert - which
> > was lucky since he discovered another embarrasing mistake - my suggestion
> > would be to change it to a while loop instead (see attached patch).
> >
> > Best regards,
> >
> > /Pär
> >
> > 2015-05-19 20:04 GMT+02:00 Hubert Tarasiuk <address@hidden>:
> > > > From: Pär Karlsson <address@hidden>
> > > >
> > > > A number of Perl-specific cleanups in the test suite perl modules.
> > > >
> > > > Most of these changes have no immediate functional difference but
> puts
> > >
> > > code
> > >
> > > > in line with the same formatting (perltidy GNU style) for
> readability.
> > > >
> > > > A warning regarding invalid operators on incompatible values
> (numeric vs
> > > > strings) have been fixed.
> > > >
> > > > Some common but discouraged Perl idioms have been updated to a
> somewhat
> > > > clearer style, especially regarding evals and map() vs. for loops.
> > > >
> > > > I did not touch the FTP and HTTP server modules functionally, but
> there
> > >
> > > seem
> > >
> > > > to be a lot of strange code there in, and would warrant further
> cleanups
> > > > and investigations if these tests would remain in Perl, instead of
> > > > moving
> > > > over to the Python based tests.
> > > >
> > > > All tests work nominally on my machine (with perl-5.20.1) but should
> be
> > > > compatible with versions down to 5.6.
> > >
> > > I believe I have found an issue with WgetTests module:
> > > >     my $i;
> > > >
> > > >     # for ($i=0; $i != $min; ++$i) {
> > > >     for my $i (0 .. $min - 1)
> > > >     {
> > > >
> > > >         last if substr($expected, $i, 1) ne substr $actual, $i, 1;
> > > >         if (substr($expected, $i, 1) eq q{\n})
> > > >         {
> > > >
> > > >             $line++;
> > > >             $col = 0;
> > > >
> > > >         }
> > > >         else
> > > >         {
> > > >
> > > >             $col++;
> > > >
> > > >         }
> > > >
> > > >     }
> > > >     my $snip_start = $i - ($SNIPPET_SIZE / 2);
> > > >     if ($snip_start < 0)
> > > >     {
> > > >
> > > >         $SNIPPET_SIZE += $snip_start;    # Take it from the end.
> > > >         $snip_start = 0;
> > > >
> > > >     }
> > >
> > > I am no Perl expert but I tested it and current for loop will not set
> > > the $i variable to any value (outside the loop). (Actually it gives me
> > > warning about using uninitialized variable when calculating $snip_start
> > > and the diff is not printed correctly.)
> > > Reverting the commented version seems to fix this problem. Removing the
> > > "my" from current for loop does not seem to fix the problem.
> > > --
> > > Hubert Tarasiuk
>