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

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Testing Suite
--
enhancement
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Dennis Melentyev, Assigned: Dennis Melentyev)

Tracking

2.20
Bugzilla 2.20
Bug Flags:
approval +
approval2.22 +
approval2.20 +

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

13 years ago
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
(Assignee)

Comment 1

13 years ago
Created attachment 199163 [details] [diff] [review]
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

13 years ago
I agree, this is a good idea.
Assignee: zach → nobody
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → 2.20
(Assignee)

Comment 3

13 years ago
Created attachment 199298 [details] [diff] [review]
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
(Assignee)

Updated

13 years ago
Attachment #199163 - Attachment is obsolete: true
Attachment #199298 - Flags: review?(mkanat)
(Assignee)

Updated

13 years ago
Assignee: nobody → dennis.melentyev
(Assignee)

Updated

13 years ago
Attachment #199298 - Flags: review?(zach)
Attachment #199298 - Flags: review?(mkanat)
Attachment #199298 - Flags: review?(gerv)
(Assignee)

Updated

13 years ago
Attachment #199298 - Flags: review?(zach)
Attachment #199298 - Flags: review?(jouni)
Attachment #199298 - Flags: review?(gerv)
(Assignee)

Comment 4

13 years ago
Created attachment 199412 [details] [diff] [review]
Splitted patch. Testcode only

Bug splitted in two parts: This one for runtests suite update and bug 312307
for current errors in tag usage
Attachment #199298 - Attachment is obsolete: true
Attachment #199412 - Flags: review?(jouni)
(Assignee)

Updated

13 years ago
Attachment #199298 - Flags: review?(jouni)
(Assignee)

Updated

13 years ago
Depends on: 312307

Comment 5

13 years ago
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.
Attachment #199412 - Flags: review?(jouni) → review-
(Assignee)

Comment 6

13 years ago
Created attachment 200102 [details] [diff] [review]
Rewised patch

Patch rewised according to Jouni's comments
Attachment #199412 - Attachment is obsolete: true
Attachment #200102 - Flags: review?(jouni)
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Whiteboard: Patch awaiting review
(Assignee)

Comment 7

13 years ago
Created attachment 207415 [details] [diff] [review]
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)
Attachment #200102 - Attachment is obsolete: true
Attachment #207415 - Flags: review?(LpSolit)
Attachment #200102 - Flags: review?(jouni)
(Assignee)

Comment 8

13 years ago
Created attachment 207511 [details] [diff] [review]
Per module reporting

Reworked again:
1. Added per module/template error reporting
2. Reduced template file processing overhead
3. Overall code readability improvements
Attachment #207415 - Attachment is obsolete: true
Attachment #207511 - Flags: review?(LpSolit)
Attachment #207415 - Flags: review?(LpSolit)

Updated

13 years ago
Target Milestone: --- → Bugzilla 2.20

Comment 9

13 years ago
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! :)
Attachment #207511 - Flags: review?(LpSolit) → review-

Updated

13 years ago
Whiteboard: Patch awaiting review
(Assignee)

Comment 10

13 years ago
Created attachment 213606 [details] [diff] [review]
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')
Attachment #207511 - Attachment is obsolete: true
Attachment #213606 - Flags: review?(LpSolit)

Comment 11

13 years ago
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
Attachment #213606 - Attachment description: Fixed issues pointed by Frederick → Fixed issues pointed by Frederic
Attachment #213606 - Flags: review?(LpSolit) → review+

Comment 12

13 years ago
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.
Flags: approval?
Flags: approval2.22?
Flags: approval2.20?

Comment 13

13 years ago
Created attachment 213625 [details] [diff] [review]
add missing error tag in code-error.html.tmpl in 2.20
Attachment #213625 - Flags: review?(wicked)
Attachment #213625 - Flags: review?(wicked) → review+
Flags: approval?
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+

Comment 14

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

13 years ago
Thanks, Frederic!
You need to log in before you can comment on or make changes to this bug.