312042 - Testing suite (runtests.pl) need to check error tags mess

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, enhancement)

Description

[u197037]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217
Build Identifier: N/A

runtests.pl does not check for unused tags and does not check for used but
undefined ones.

Reproducible: Always



Expected Results:  
Report about misused error tags

Comment 1

[u197037]

Attachment: Sample patch for goodperl.t

This patch is definitely not the think we should put into codebase, but rather
a sample of desired result.
Please, do not assign me on this bug now - I would not have enough time for it.

Comment 2

[Max Kanat-Alexander]

I agree, this is a good idea.

Comment 3

[u197037]

Attachment: First reliable patch. Including error templates/scripts fixes

This is cumulative patch, it fixes unused/misused tags in code and in
templates.
Also, adds 012throwable.t test set

Comment 4

[u197037]

Attachment: Splitted patch. Testcode only

Bug splitted in two parts: This one for runtests suite update and bug 312307
for current errors in tag usage

Comment 5

[Jouni Heikniemi]

Comment on attachment 199412 [details] [diff] [review]
Splitted patch. Testcode only

All right... I get the idea of this patch and feel it's good, but I also have
severe trouble reading it. Here are some suggestions on enhancing it:

>+# Collect all used tags from cgi/pm files

First of all, I'm not sure "tag" is the correct term here, although I do see
your point. Re a short discussion on IRC, I think it would be best to just talk
about "errors" (as the identifiers that resolve into error messages in the
xxx-error templates).

>+    if ($path =~ /Error.pm$/) {
>+        # Special case for Template error handling
>+        $CodeErrors{template_error} = "Bugzilla/Errors.pm";
>+        next;
>+    }

Special case what? How? I can see it's a special case, but how? This also
evidently points that "CodeErrors" is a bit ambiguous. What does it mean to
push a file into an hash called "CodeErrors"? 

Also note that your regex will match things such as "ErrorsRpm" (the . needs
quoting) and "NoError.pm" (you're not checking for a path separator before it).

>+    while (my $path_line = <TMPL>) {

"path_line"? These are lines of content which have nothing to do with "paths",
right? (repeat later on when reading the templates)

>+        if ($path_line =~ /Throw.*Error\s*\(\s*["'](.*?)["']/) {

As a rule, you shouldn't use "Throw.*Error" if you're sure you'll only parse
"Code|User" there. Although a failure case for that regex would probably never
exist, spell it out so that the regex remains more readable.

>+                while (my $path_line = <TMPL>) {
>+                    $lineno++;
>+                    if ($path_line =~ /error\s*==\s*"(.*?)"/) {
>+                        my $errtag = $1;
>+                        if ($errtag =~ /\s/) {
>+                            ok(0,"$path has a Throw*Error call on line $lineno which doesn't use a tag --ERROR");
>+                            $error = 1;       
>+                        }

Uh... what scenario does this cover for? 

>+                            if (defined $CodeErrors{$errtag}) {
>+                                $TmpCodeErrors{$errtag} = "";
>+                            }

You know, this business with CodeErrors and TmpCodeErrors is really confusing
:-)

>+                                ok(0,"$path has an unused Throw*Error tag '$errtag' text on line $lineno --WARNING");

"$path has an unused code error called '$errtag' on line $lineno" (or something
like that)

Since you're branching the codeerror and usererror paths here, dispense with
the Throw*Error cruft and just use the real stuff.

>+                ok(1,"$path uses Throw*Error tags correctly") if !$error;

Does it make any sense to report this for the template files anyway? It's not
like they really "used" things separately from the code base...

>+            ok(0,"Tag '$errtag' used in '". $CodeErrors{$errtag} . "' but not
>+            defined in code-errors.html.tmpl for $lang l10n");

This is ugly:

not ok 14 - Tag 'group_id_invalid' used in 'post_bug.cgi: 329, ' but not
	    defined in code-errors.html.tmpl for en l10n

You might want to fix it by collecting the usage sites into an array instead of
a comma-separated string and then printing it with some readable formatting.


AFAICS, apart from the couple of corner cases mentioned above, I'd think the
patch works as expected.

Comment 6

[u197037]

Attachment: Rewised patch

Patch rewised according to Jouni's comments

Comment 7

[u197037]

Attachment: Updated patch

Updated according to Frederik's comments on IRC:
1. elimitaned unneeded language loop
2. checking for the begining of POD looking for /^\s*__END__/
3. unescaped dots now got a backslashes
4. qq's gone in fawour of double quotes
5. added success reporting (but per error tag, not per cgi/pm)

Comment 8

[u197037]

Attachment: Per module reporting

Reworked again:
1. Added per module/template error reporting
2. Reduced template file processing overhead
3. Overall code readability improvements

Comment 9

[Frédéric Buclin]

Comment on attachment 207511 [details] [diff] [review]
Per module reporting

>+# Bug 137589 - Disable command-line input of CGI.pm when testing
>+use CGI qw(-no_debug);

Nit: from what I read, this line was added due to older versions of CGI.pm, such as version 2.56. But now that we require at least version 2.93, this is not a problem anymore. Btw, only 004template.t uses it.


>+# Find all modules
>+foreach my $module (@Support::Files::testitems) {
>+    $test_modules{$module}{errors} = ();
>+}

Unless I missed something, $test_modules{$module} is always followed by the 'errors' key. No other key is used at this position. So {errors} doesn't bring any useful information; it just makes the hash longer and harder to read. You should remove it.


>+# Set requested tests counter.
>+eval("use Test::More tests => $tests");

Instead of eval(), write:

use Test::More;

at the beginning of the script, and write:

plan tests => $tests;

here. That's what the POD for Test::More suggests.


>+# Collect all errors defined in templates
>+foreach my $file (keys %test_templates) {
>+    $file =~ m|template/([^/]+)/.*global/([^/]+)-error\.html\.tmpl|;

Nit: shouldn't it be "/[^/]+/global/" instead of ".*global/" ? Else you would catch "notglobal/user-error.html.tmpl" too.


>+        if ($line =~ /\[%\s[A-Z]*\s*error\s*==\s*"(.*?)"\s*%\]/) {

Nit: You should write [A-Z]+ instead of [A-Z]*. Also, I would write (.+) instead of (.*?).


>+            else {
>+                AddDefined($errtype, $errtag, $lang, $file, $lineno);
>+            }

AddDefined() is a one-liner and is used only once. Please remove this function and write its content here. Having to scroll up and down to understand what this routine does is painful.


>+    while (my $line = <TMPL>) {
>+        last if $line =~ /^\s*__END__/; # skip the POD (at least in

Nit: AFAIK, __END__ is always at the beginning of a line, so you can remove \s*.


>+        if ($line =~ /Throw(Code|User)Error\s*\(\s*["'](.*?)['"]/) {

You should ignore comments. You should write:
if ($line !~ /^\s*#/ && $line =~ /Throw(Code|User)Error\s*\(\s*["'](.*?)['"]/)

Catching POD is more difficult, though (that's why I like having POD after __END__).


>+            AddUsed($errtype, $errtag, $file, $lineno);

Same comment as before. AddUsed is a one-liner and is used only once.


>+sub AddUsed {
>+    my ($errtype, $errtag, $file, $lineno) = @_;
>+    push @{$Errors{$errtype}{$errtag}{used_in}{$file}}, $lineno;
>+}

One-liner and used only once = bye bye!


>+sub AddDefined {
>+    my ($errtype, $errtag, $lang, $file, $lineno) = @_;
>+    push @{$Errors{$errtype}{$errtag}{defined_in}{$lang}{$file}}, $lineno;
>+}

Same comment here.


>+sub Register {
>+    my ($hash, $file, $message) = @_;
>+    push @{$hash->{$file}{errors}}, $message;
>+}

Nit: Here too, if the only key we will ever have is 'errors', then this key is useless and should be removed, for clarity.


>+    else {
>+        ok(1, "$file");
>+    }

This is a bit short. You should write "$file uses no undefined error tags".


I tested your patch on 2.23. It seems to work great! :)

Comment 10

[u197037]

Attachment: Fixed issues pointed by Frederic

There are couple of differences to your suggestions. The most arguable is 
  if ($line =~ /[^#]*Throw(Code|User)Error\s*\(\s*["'](.*?)['"]/) {
instead of proposed 
  if ($line !~ /^\s*#/ && $line =~ /Throw(Code|User)Error\s*\(\s*["'](.*?)['"]/) {

mine should not false trigger on line like this (I hope, not tested yet actualy):

 my var = some->code(); # || ThrowCodeError('blah_blah')

Comment 11

[Frédéric Buclin]

Comment on attachment 213606 [details] [diff] [review]
Fixed issues pointed by Frederic

>+        if ($line =~ /[^#]*Throw(Code|User)Error\s*\(\s*["'](.*?)['"]/) {

You have to write $line =~ /^[^#]* <--- note the ^ before [].


>+sub DefinedIn {
>+    my ($errtype, $errtag, $lang) = @_;
>+    my $message = "$errtype error tag '$errtag' is defined in:\n";

$message isn't used.


Both should be fixed on checkin. r=LpSolit

Comment 12

[Frédéric Buclin]

Note that we will light the tree on 2.20:
'invalid_numeric_argument' is used in Bugzilla/User.pm but isn't defined.

2.18.5 is full of undefined error tags too.

Comment 13

[Frédéric Buclin]

Attachment: add missing error tag in code-error.html.tmpl in 2.20

Comment 14

[Frédéric Buclin]

tip:

RCS file: /cvsroot/mozilla/webtools/bugzilla/t/012throwables.t,v
done
Checking in t/012throwables.t;
/cvsroot/mozilla/webtools/bugzilla/t/012throwables.t,v  <--  012throwables.t
initial revision: 1.1
done


2.22rc1:

Checking in t/012throwables.t;
/cvsroot/mozilla/webtools/bugzilla/t/012throwables.t,v  <--  012throwables.t
new revision: 1.1.2.2; previous revision: 1.1.2.1
done


2.20.1:

Checking in t/012throwables.t;
/cvsroot/mozilla/webtools/bugzilla/t/012throwables.t,v  <--  012throwables.t
new revision: 1.1.4.2; previous revision: 1.1.4.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.52.2.5; previous revision: 1.52.2.4
done

Comment 15

[u197037]

Thanks, Frederic!