action #10524
closed[functional][u] Prevent use of potentially uninitialized values
0%
Description
Motivation¶
Prevent sneaky errors confusing users by ignoring that values are unitialized.
Acceptance criteria¶
- AC1: Static code checks or unit tests in the tests distribution prevent the use of uninitialized values
Observation¶
https://openqa.suse.de/tests/195260/file/autoinst-log.txt
Use of uninitialized value $1 in numeric eq (==) at tests/console/zypper_up.pm line 23.
...
Use of uninitialized value $1 in numeric eq (==) at tests/console/zypper_up.pm line 28.
steps to reproduce¶
probably rerun these tests
problem¶
There is no check for uninitialized values in tests which could cause unforeseen results.
suggestion¶
Prevent use of potentially uninitialized values by static code checks in tests distribution.
Updated by asmorodskyi almost 8 years ago
- Status changed from New to In Progress
Updated by asmorodskyi almost 8 years ago
- Status changed from In Progress to Rejected
zypper_up not exists anymore so ticket is not relevant
Updated by okurz almost 8 years ago
- Status changed from Rejected to New
Updated by okurz almost 7 years ago
- Subject changed from Prevent use of potentially uninitialized values to [functional]Prevent use of potentially uninitialized values
- Target version set to Milestone 17
Updated by okurz over 6 years ago
- Target version changed from Milestone 17 to Milestone 21+
Updated by okurz over 6 years ago
- Target version changed from Milestone 21+ to Milestone 21+
Updated by okurz over 6 years ago
- Subject changed from [functional]Prevent use of potentially uninitialized values to [functional][u] Prevent use of potentially uninitialized values
Updated by okurz about 6 years ago
- Description updated (diff)
- Status changed from New to Workable
- Target version changed from Milestone 21+ to Milestone 22
Updated by agraul about 6 years ago
- Status changed from Workable to In Progress
- Assignee set to agraul
Updated by agraul about 6 years ago
I tested if use strict;
prevents the use of uninitialized values and from my small experiment it does:
no warnings, no strict:
#!/usr/bin/perl
sleep(3);
print $foo if ($foo < 3);
-> no output,
only warnings:
#!/usr/bin/perl
use warnings;
sleep(3);
print $foo if ($foo < 3);
Output after the three seconds sleep have passed:
Use of uninitialized value $foo in numeric lt (<) at strict.pl line 6.
Use of uninitialized value $foo in print at strict.pl line 6.
Only strict:
#!/usr/bin/perl
# use warnings;
use strict;
sleep(3);
print $foo if ($foo < 3);
Output (before executing the sleep):
Global symbol "$foo" requires explicit package name (did you forget to declare "my $foo"?) at strict.pl line 6.
Global symbol "$foo" requires explicit package name (did you forget to declare "my $foo"?) at strict.pl line 6.
Execution of strict.pl aborted due to compilation errors.
Conclusion:
use strict
prevents the case that we want to solve with this ticket.
Next I enabled the check in make perlcritic
(--include "strict"
), this gives me to following numbers (executed inside os-autoinst-distri-opensuse): 923 modules that fail due to missing use strict;
or use warnings;
(perlcritic checks for both), 173 modules pass the test.
Updated by okurz about 6 years ago
I don't understand why the check would report that so many modules would miss strict.
git ls-files '*.pm' | xargs grep -L 'use \(strict\|5.018\)'
gives me just a very limited list:
lib/LTP/TestInfo.pm
lib/publiccloud/azure.pm
lib/publiccloud/ec2.pm
lib/publiccloud/instance.pm
lib/publiccloud/provider.pm
so what's going on?
Updated by agraul about 6 years ago
okurz wrote:
I don't understand why the check would report that so many modules would miss strict.
git ls-files '*.pm' | xargs grep -L 'use \(strict\|5.018\)'
gives me just a very limited list:
lib/LTP/TestInfo.pm
lib/publiccloud/azure.pm
lib/publiccloud/ec2.pm
lib/publiccloud/instance.pm
lib/publiccloud/provider.pmso what's going on?
I used perlcritic --include strict -l
to create the list, which also checks for use warnings;
- Is there a good reason not to enable "warnings"? If so, I will look into singling out the rule for strict, else I will just update the Makefile to include --inlcude "strict"
in make perlcritic
Updated by okurz about 6 years ago
I see. Yes, I think use warnings
should also be used in all files except where we use something like use 5.018
or use 5.12
(see https://stackoverflow.com/a/6050356). I don't know what is the best approach. But enabling tests sounds good but only when combined with an actual fix. That however should not be hard with a right combination of "git grep" and "sed" to change existing files.
Updated by agraul about 6 years ago
- Status changed from In Progress to Feedback
PR can be found at: https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/6586
Updated by agraul about 6 years ago
- Blocked by action #47012: [functional][u] Summary or Maintainer info missing from test modules added
Updated by agraul about 6 years ago
The mentioned PR adds 'use strict' and 'use warnings' to all test modules and libraries, except for "lib/mail_ltp.pm". This is due to repeated evaluations of said library, which cause "redefined subroutines". Therefore the file is not using warnings and it is excluded from the perlcritic "strict" check.
I came across another issue: Some of the files don't have maintainer or summary information in them. Touching them caused our CI to check for those pieces of information, exposing the lack of them and failing the test.
I created #47012 as a blocker to address that issue.
Updated by okurz about 6 years ago
- Due date set to 2019-03-26
- Assignee set to okurz
No "Blocked" without assignee, assigning self after I scheduled #47012 for soon
Updated by okurz about 6 years ago
- Target version changed from Milestone 22 to Milestone 23
Updated by okurz almost 6 years ago
- Status changed from Blocked to Feedback
Based on https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/6877 I created https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/6586 which is including all commits by alexandergraul plus the additional needed changes, e.g. maintainer adjustements
Updated by okurz almost 6 years ago
- Status changed from Feedback to Resolved
All good for now after I pushed two more minor commits directly to master as my PR was merged while in the meantime some modules where introduced that missed "use warnings;" which I subsequently added. I am not exactly sure if #10524#note-11 is true for us in all cases as we dynamically evaluate the test modules only during test execution but I still think this change should help us. Let's just call it done and handle specific other issues as soon as we hit them.