Closed Bug 337444 Opened 18 years ago Closed 18 years ago

t/012throwables.t shouldn't fail due to unused error tags in code/user-error.html.tmpl

Categories

(Bugzilla :: Testing Suite, defect)

2.20.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

Attachments

(1 file, 3 obsolete files)

If you write:

my $tag = "foo";
ThrowCodeError($tag, $vars);

and in code-error.html.tmpl you have:
[% ELSIF error == "foo" %]

t/012throwables.t will complain that 'foo' in unused and will fail. As the code is not broken (and in some cases is only unused code which could be safely removed), it should return ok(1, --WARNING unused error tag 'foo') instead of ok(0, ) so that tests are still successful in this case.
Attached patch patch for tip (obsolete) — Splinter Review
I'm not sure to change entire output so I added an argument.

add dummy tag and tested on local(win32), below is the output
perl runtests.pl 12 -v
...
ok 118 - template/en/default/global/user-error.html.tmpl has 1 error(s):
# user error tag 'account_inexisten' is defined at line(s) (75) but is not used anywhere
ok
All tests successful.
Files=1, Tests=118,  6 wallclock secs ( 0.00 cusr +  0.00 csys =  0.00 CPU)
Attachment #221596 - Flags: review?
Comment on attachment 221596 [details] [diff] [review]
patch for tip

> sub Report {
>+    my ($test_type, $file, @errors) = @_;
>     if (scalar @errors) {
>+        if($test_type eq 'tmpl'){
>+            ok(1, "$file has ". scalar @errors ." error(s):\n" . join("\n", @errors));
>+        }else{
>+            ok(1, "--WARNING $file has unused error tag(s)". scalar @errors ." error(s):\n" . join("\n", @errors));
>+        }
>     }

Wrong! We still have to fail if $test_type eq 'module'. --WARNING is for $test_type eq 'tmpl'.
Moreover, 'module' should be 'unknown_tag' and 'tmpl' should be 'unused_tag'.
Attachment #221596 - Flags: review? → review-
Assignee: zach → bmo
Attached patch patch for tip v2 (obsolete) — Splinter Review
fix arg name and silly mistake
output:
ok 118 - --WARNING template/en/default/global/user-error.html.tmpl has unused er
ror tag(s)1 error(s):
# user error tag 'account_inexisten' is defined at line(s) (75) but is not used anywhere
ok
All tests successful.
Files=1, Tests=118,  7 wallclock secs ( 0.00 cusr +  0.00 csys =  0.00 CPU)
Attachment #221596 - Attachment is obsolete: true
Attachment #221603 - Flags: review?
Comment on attachment 221603 [details] [diff] [review]
patch for tip v2


> # Now report templates results
> foreach my $file (sort keys %test_templates) {
>-    Report($file, @{$test_templates{$file}});
>+    Report('unused_tag', $file, @{$test_templates{$file}});
> }

There is still one problem. If you have several errors for a given file and one is critical, it shouldn't return ok(1, ). To test this, try adding an error tag containing a whitespace "foo bar". Talking about this, there is a missing whitespace in the error message "withspace" -> "with space".

We have to find a way to distinguish critical from non-critical errors in code/user-error-html.tmpl. The only non-critical error is unused tags, all others should be considered critical (such as having whitespaces in error tag names).
Attachment #221603 - Flags: review? → review-
Attached patch patch for tip v3 (obsolete) — Splinter Review
using %test_templates_error to catch
Attachment #221603 - Attachment is obsolete: true
Attachment #221625 - Flags: review?
Attached patch patch, v4Splinter Review
_RSZ_'s patch duplicated too much code. Hashes allow us to be much more compact.
Assignee: bmo → LpSolit
Attachment #221625 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #221801 - Flags: review?(mkanat)
Attachment #221625 - Flags: review?
Comment on attachment 221801 [details] [diff] [review]
patch, v4

Yep, works for me.

Now we have to fix the actual bug of 012throwables not noticing that certain error tags are AUTH_ERROR codes.
Attachment #221801 - Flags: review?(mkanat) → review+
Flags: approval?
Blocks: 337701
This test should behave the same way on all branches. Requesting approval for 2.20 and 2.22 too.
Flags: approval2.22?
Flags: approval2.20?
Flags: approval?
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
tip:

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

2.22:

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

2.20.2:

Checking in t/012throwables.t;
/cvsroot/mozilla/webtools/bugzilla/t/012throwables.t,v  <--  012throwables.t
new revision: 1.1.4.4; previous revision: 1.1.4.3
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.