Closed Bug 1137674 Opened 8 years ago Closed 8 years ago

Merge t/003safesys.t with t/001compile.t


(Bugzilla :: Testing Suite, enhancement)

Not set



Bugzilla 6.0


(Reporter: LpSolit, Assigned: LpSolit)




(1 file)

Both t/001compile.t and t/003safesys.t are doing *exactly* the same thing: loop over all files in @testitems and compile them, 001 to detect if the file compiles correctly, and 003 to detect if it uses system() and exec() correctly. In both cases must the file be compiled, the only difference being that 001 looks for "syntax OK" while 003 looks for "arguments for Support::Systemexec::(system|exec)" in the returned value. This is a waste of time.

Moreover, 001compile.t is much more clever than 003safesys.t to determine which files to compile or not, based on installed Perl modules (optional features). Also, 001compile.t supports $ENV{PERL5LIB}. 003safesys.t doesn't. So we should kill 003safesys.t and let 001compile.t do all the compilation work. This would automatically fix bug 1116118 on master.
Attached patch patch, v1Splinter Review
Attachment #8570454 - Flags: review?(dylan)
In case you are wondering, 001compile.t has a shortcut for .pm files: it calls use_ok() instead of invoking the perl interpreter, because this is much faster. The downside is that .pm files are not loaded with -MSupport::Systemexec, i.e. 001compile.t doesn't check the number of arguments passed to system() and exec() in them. But this is not really a regression, because 003safesys.t is unable to detect system() and exec() in .pm files anyway. Said differently, 003safesys.t only works as designed with .cgi and .pl scripts. This is why I left the shortcut in 001compile.t.

13 years that these tests are broken, they never worked, and nobody ever noticed it. /me blames zach (bug 97976 comment 11). ;)
Comment on attachment 8570454 [details] [diff] [review]
patch, v1

Review of attachment 8570454 [details] [diff] [review]:

yes, kill it. kill it with fire.

// technically, if you wanted to do the Systemexec hack (inject symbols into every namespace) we can do that. I don't think we should do that though.
Attachment #8570454 - Flags: review?(dylan) → review+
Flags: approval?
Flags: approval? → approval+
To ssh://
   2ccf81d..56d130c  master -> master
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.