324014 - Testing suite (runtests.pl) needs to check Bugzilla->params usage to assure valid params

Status: RESOLVED WONTFIX

(Bugzilla :: Testing Suite, enhancement, P2)

Description

[u197037]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.8) Gecko/20051219 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.8) Gecko/20051219 Firefox/1.5

runtests.pl does not check for unused/misused parameters accessed via Param() method.

Reproducible: Always

Steps to Reproduce:
run ./runtests.pl
Actual Results:  
None

Expected Results:  
Report about misused parameters

Comment 1

[u197037]

Attachment: Initial version of patch

Frederik, Could you please confirm the bug, reassign it to me and review the patch?

Comment 2

[Frédéric Buclin]

Comment on attachment 208967 [details] [diff] [review]
Initial version of patch

run your test on tip and you will notice a full list of errors.

Moreover, it doesn't catch this syntax, see process_bug.cgi:

    my $ret = Param( "commenton" . $function );

Comment 3

[u197037]

(In reply to comment #2)
...
> Moreover, it doesn't catch this syntax, see process_bug.cgi:
> 
>     my $ret = Param( "commenton" . $function );
Well I have no idea of how to test that. In order to do that, I will need also check for all possible values of $function.

M.b. I do need some sort of exceptions :/

Also, handling of rows like this one:
if (Param("mail_delivery_method") eq "sendmail" && !Param("sendmailnow"))
is quite complicated.

But in general, does the thing work right way?

Comment 4

[u197037]

Attachment: Revised version

Revised version to process multiple Param("parname") functions in one line.
Added processing of files, excluded in %Support::Files::exclude_deps
Added hacks to not mention hackish processing of commenton* parameters in process_bug.cgi

Comment 5

[u197037]

Attachment: reworked error level handling

Reworked variant:
Added non-critical warnings about [probably] unused parameters, usage of parameters with variables in their names including ones in templates.

Comment 6

[Frédéric Buclin]

I didn't look at the script in great details yet, but here are some remarks already:

- Results about files should be displayed as soon as a file has been analyzed instead of waiting that all files have been analyzed. That's the way all other test scripts work (and this also avoid a long delay with no output).

- Most subroutines are called only once and some of them are one-liners. This doesn't make sense to me, this is confusing and makes the code harder to read. Please move the code inline.

Comment 7

[Frédéric Buclin]

Comment on attachment 209714 [details] [diff] [review]
reworked error level handling

per my previous comment

Comment 8

[Frédéric Buclin]

Two more things:

- __END__ always is at the beginning of the line, so the regexp should look for /^__END__/

- Be sure to correctly filter comments. # Param('inexistant_param') should not be detected as it's in a comment.

Comment 9

[u197037]

Attachment: Fred's comments fixed

Fixed things:
1. /^__END__/
2. eval Test::More -> plan tests => $tests
3. CGI commandline hack removed
4. one-liners eliminated
5. ok(1,$file) got more text
6. comments got stripped before search for Param("blah")
7. Anything else I forgot?

Comment 10

[Frédéric Buclin]

Comment on attachment 213739 [details] [diff] [review]
Fred's comments fixed

>+use Bugzilla::Constants;
>+use Bugzilla::Config qw(:DEFAULT :admin :params $datadir);
>+use Bugzilla::Config::Common;
>+use vars qw(@parampanels);

You only need :params from Config.pm. You don't the other lines or arguments.


>+# Hackaround to add files excluded by Support::Files::exclude_deps back
>+my @to_add = ();
>+foreach my $file (values %Support::Files::exclude_deps) {
>+    push @to_add, @$file;
>+}

Do not include excluded files, by definition. Tests should be based on what the installation is running. If you want to include all files, then install all required Perl modules.


>+# Define files to test. Each file would have a list of error messages, if any.
>+my @testitems = @Support::Files::testitems;
>+for my $path (@Support::Templates::include_paths) {
>+   push(@testitems, map(File::Spec->catfile($path, $_),
>+                        Support::Templates::find_actual_files($path)));
>+}

Shouldn't you use $include_path{$langdir} as in 004template.t? Else you are scanning the template/ directory twice, aren't you?


>+# Find Param() usage
>+my %results = ();
>+foreach my $file (@testitems) {

IMO, you should scan files in the usual way, i.e. as other tests do (and not as you did in 012throwables.t).


>+    # Check for of undefined params in use
>+    foreach my $param (keys %file_params) {
>+        if (!defined $params{$param}{defined_in}) {
>+            $results{$file}{errtype} = $errtype;
>+            my $message = "--ERROR: ";
>+            $message = "--WARNING: Possible use of variables! Check this manually!\n"
>+                if $errtype;

I noted that if one parameter gets a WARNING because it's a variable and another one gets an ERROR because it's undefined, then both parameters get an ERROR.


>+sub Report {
>+    my ($file, $errtype, @errors) = @_;
>+    if (scalar @errors) {
>+        ok($errtype, "$file has ". scalar @errors ." " . ($errtype ? "warning" : "error") . "(s):\n" . join("\n", @errors));

If you report errors and warning as soon as they are detected, you probably don't need this routine.

Moreover, as you can use variables in Param(), you shouldn't report unsused parameters from Config/*.pm modules. This is confusing (e.g. all these commentonfoo parameters used in process_bug.cgi). IMO, this script should *only* report undefined parameters, not unused ones. This would simplify your test script a bit.

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> You only need :params from Config.pm. You don't the other lines or arguments.

Missing word: You don't *need* the other lines...

Comment 12

[Frédéric Buclin]

Only security and dataloss fixes will be accepted on the 2.20 branch.

Comment 13

[Frédéric Buclin]

Note that you have no chance now to catch invalid params as we sometimes write:

my $changes = Bugzilla->params;
my $foo = $params->{foo};

and so you won't be able to catch $params->{foo} as you have no way to know what $params is.

Comment 14

[u197037]

re-assign to default, since there were no time to work on Bugzilla in last two years, sorry.

Comment 15

[Frédéric Buclin]

WONTFIX per comment 13.