The @Support::Templates::testitems module variable does not list every template stored under the templates/default/ directory. The Support::Templates module should probably use a glob to pick up all of the files rather than pattern- matching through perl scripts. I will attach a patch to t/004template.t that demostrates this by checking for version strings in all template files using a glob() instead of @Support::Templates::testitems. The following templates were found not to have version strings (I'll open another bug for this later): buglist/buglist-rdf.rdf.tmpl buglist/buglist-simple.html.tmpl buglist/buglist.html.tmpl buglist/change-form.tmpl buglist/table.tmpl prefs/account.tmpl prefs/email.tmpl prefs/footer.tmpl prefs/permissions.tmpl show/navigate.html.tmpl

Target: 2.16. Add CCs. Assigning to me.

Modifies t/004template.t to use a glob() instead of @Support::Templates::testitems to test for presence of version info at the top of template files. Only a test case, not a patch to fix the issue.

Doesn't that miss the header, footer, and files (like index) at the root level?

The test case (which I never claimed to be complete :^) does grab index.tmpl, but misses global/header and global/footer. I think I'm going to try using File::Find instead to create a list of template filenames. Will we be "fixing" (standardizing) the names of templates for the 2.16 release? We've got *.TYPE.tmpl, *.tmpl, *.atml and files with no suffix currently in use as templates. This bug is also related to Bug 130254. Added CCs.

Yes, we'll be standardizing on the following style before we release 2.16: NAME-WITH-HYPHENS.EXTENSION.tmpl -myk

Yes, we will be fixing template filenames. See Bug 135707.

Check the File::Find usage in bug 97832.

This is the first version of the patch to test all templates during t/004template.t and t/005no_tabs.t. However, it won't be the final patch since the find_templates() subroutine will be simplified in t/Support/Templates.pm once Bug 135707 goes through. I want to get feedback on the other changes before then, though. t/004template.t - Cleaned up BEGIN{} usage. - Changed first foreach() test loop to use @referenced_files, which is the original value of @testitems from Support::Templates. - Added more dummy filters to Template object (to speed up testing a bit). - Changed remaining foreach() loops to use new @actual_files array from Support::Templates to make sure we test all of the template files for compilation errors and version strings. - Used File::Spec methods where appropriate to aid in making Bugzilla work on non-UNIX systems. t/005no_tabs.t - Cleaned up BEGIN{} usage. - Changed @testitems to @actual_files from Support::Templates module. - Used File::Spec methods where appropriate to aid in making Bugzilla work on non-UNIX systems. - Changed foreach() loop to use less memory and run more quickly by not reading in the entire file first for tab testing. t/Support/Templates.pm - use strict - Renamed @testitems to @referenced_files. - Added @actual_files array that files all template files under template/default using the File::Find module and the find_templates() subroutine. - Used File::Spec methods where appropriate to aid in making Bugzilla work on non-UNIX systems.

Should I file a bug (for 2.18 or later) to include templates from the template/custom directory in the testing process so users that have custom templates may test them?

When we precompile, checksetup will iterate along template/default, but will end up testing template/custom (and any templates they refer to) because of the include path.

NOTE: Patch v.1 (attachment 78232 [details] [diff] [review]) will become invalid once patch for Bug 137589 is checked in.

Adding dependency to Bug 136180 since it will add a dummy 'url_quote' filter to t/004template.t. Patch v.1 will need to be updated.

Comment on attachment 78232 [details] [diff] [review] Patch v.1 Marking needs-work since we're waiting on other patches to land first.

OK, we should be clear to do this now. Gerv

- Adds 'uri' stub filter to t/004template.t (oops...accidentally left that out of Bug 136180 patch) - Simplifies test for filter filenames in t/Support/Templates.pm

Comment on attachment 80827 [details] [diff] [review] Patch v.2 >+ html => sub { return $_ } , > js => sub { return $_ } , > strike => sub { return $_ } , >+ uri => sub { return $_ } , > url_quote => sub { return $_ } , I don't get why you are doing this - built-in TT filters don't need to be mentioned here. And uri and html are built-in. I don't understand the tests well enough to be confident of giving this review :-( Gerv

> I don't get why you are doing this - built-in TT filters don't need > to be mentioned here. And uri and html are built-in. I did this to make the routines (theoretically) run quicker when doing testing. I'll go ahead and take them out since they are creating some confusion. Another patch will follow shortly.

Comment on attachment 80827 [details] [diff] [review] Patch v.2 Marking needs-work per Comment #17.

- Removed stubs in t/004template.t for 'html' and 'uri'. - Added comment to t/004template.t about the real filters being defined in globals.pl. - Added a comment in globals.pl about adding a stub filter to t/004template.t when adding a new filter that doesn't override a built-in TT filter.

Comment on attachment 80944 [details] [diff] [review] Patch v.3 r=gerv. Seems OK to me. Gerv

Comment on attachment 80944 [details] [diff] [review] Patch v.3 r=bbaetz. The comment you added in globals.pl is not mentioned a bit more generally above. You can probably remove it. If you think its still needed, make the comment refer to checksetup.pl, too.

s/not/now/

Comment on attachment 80944 [details] [diff] [review] Patch v.3 Rats! Bug 140664 found some short-comings in File::Find in Perl 5.00503. File::Spec seems to be missing the abs2rel() method in Perl 5.00503, so this patch probably won't work with that version of Perl. I need to test this patch on Perl 5.00503 before it is up for review again. I also violated some curly brace placement rules with respect to the developer's guide. D'oh!

We require File::Spec version 0.82 in checksetup.pl, so I thin its reasonable to require it for the tests, too.

- Fixed comment in globals.pl per comment #21 - Added version after 'use File::Spec' to t/004template.t, t/005no_tabs.t and t/Support/Templates.pm - Fixed curly brace spacing from Patch v.3 in t/Support/Templates.pm

Comment on attachment 81526 [details] [diff] [review] Patch v.4 r=gerv. Gerv

Those of you with Perl 5.005 installed, I would appreciate it if you checked Patch v.4 (attachment 81526 [details] [diff] [review]) with a clean bugzilla-tip. Run "./runtests.sh" and let me know if they all pass. (NOTE: You may need to fix the path to perl in runtests.sh.) I may be using features in File::Find that won't work under Perl 5.005. (I'm in the process of setting up a 5.00503 test environment at home, but ran out of time installing/upgrading modules over the weekend.)

Comment on attachment 81526 [details] [diff] [review] Patch v.4 You should test this on landfill, with 5.005_03, but: >+# Scan the template include path for templates then put them in >+# in the @actual_files array to be used by various tests. >+map(find({ wanted => \&find_templates }, $_), >+ split(':', $include_path)); You can't use the hash form with 5.005 - do: find(\&find_templates, $_) instead.

- Fixes calling convention of find() per Comment #28. I don't have access to landfill, but on my RH 6.2 box at home with Perl 5.00503, it seems that File::Find actually prefers a hash ref for the first argument: sub wrap_wanted { my $wanted = shift; ref($wanted) eq 'HASH' ? $wanted : { wanted => $wanted }; } So it should have worked as seen in Patch v.4. :^)

Comment on attachment 82490 [details] [diff] [review] Patch v.5 r=gerv. I'm still happy with this :-) Gerv

Comment on attachment 82490 [details] [diff] [review] Patch v.5 r= justdave

Checked in for ddk: Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.166; previous revision: 1.165 done Checking in t/004template.t; /cvsroot/mozilla/webtools/bugzilla/t/004template.t,v <-- 004template.t new revision: 1.13; previous revision: 1.12 done Checking in t/005no_tabs.t; /cvsroot/mozilla/webtools/bugzilla/t/005no_tabs.t,v <-- 005no_tabs.t new revision: 1.9; previous revision: 1.8 done Checking in t/Support/Templates.pm; /cvsroot/mozilla/webtools/bugzilla/t/Support/Templates.pm,v <-- Templates.pm new revision: 1.9; previous revision: 1.8 done