135543 - @Support::Templates::testitems does not list all templates

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, defect)

Description

[David D. Kilzer (ddk)]

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

Comment 1

[David D. Kilzer (ddk)]

Target: 2.16.  Add CCs.  Assigning to me.

Comment 2

[David D. Kilzer (ddk)]

Attachment: Test case modification to t/004template.t

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.

Comment 3

[Bradley Baetz (:bbaetz)]

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

Comment 4

[David D. Kilzer (ddk)]

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.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Yes, we'll be standardizing on the following style before we release 2.16:

NAME-WITH-HYPHENS.EXTENSION.tmpl

-myk

Comment 6

[David D. Kilzer (ddk)]

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

Comment 7

[Bradley Baetz (:bbaetz)]

Check the File::Find usage in bug 97832.

Comment 8

[David D. Kilzer (ddk)]

Attachment: Patch v.1

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.

Comment 9

[David D. Kilzer (ddk)]

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?

Comment 10

[Bradley Baetz (:bbaetz)]

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.

Comment 11

[David D. Kilzer (ddk)]

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

Comment 12

[David D. Kilzer (ddk)]

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 13

[David D. Kilzer (ddk)]

Comment on attachment 78232 [details] [diff] [review]
Patch v.1

Marking needs-work since we're waiting on other patches to land first.

Comment 14

[Gervase Markham [:gerv]]

OK, we should be clear to do this now.

Gerv

Comment 15

[David D. Kilzer (ddk)]

Attachment: Patch v.2

- 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 16

[Gervase Markham [:gerv]]

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

Comment 17

[David D. Kilzer (ddk)]

> 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 18

[David D. Kilzer (ddk)]

Comment on attachment 80827 [details] [diff] [review]
Patch v.2

Marking needs-work per Comment #17.

Comment 19

[David D. Kilzer (ddk)]

Attachment: Patch v.3

- 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 20

[Gervase Markham [:gerv]]

Comment on attachment 80944 [details] [diff] [review]
Patch v.3

r=gerv. Seems OK to me.

Gerv

Comment 21

[Bradley Baetz (:bbaetz)]

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.

Comment 22

[Bradley Baetz (:bbaetz)]

s/not/now/

Comment 23

[David D. Kilzer (ddk)]

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!

Comment 24

[Bradley Baetz (:bbaetz)]

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

Comment 25

[David D. Kilzer (ddk)]

Attachment: Patch v.4

- 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 26

[Gervase Markham [:gerv]]

Comment on attachment 81526 [details] [diff] [review]
Patch v.4

r=gerv.

Gerv

Comment 27

[David D. Kilzer (ddk)]

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 28

[Bradley Baetz (:bbaetz)]

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.

Comment 29

[David D. Kilzer (ddk)]

Attachment: Patch v.5

- 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 30

[Gervase Markham [:gerv]]

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

r=gerv. I'm still happy with this :-)

Gerv

Comment 31

[Dave Miller [:justdave]]

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

r= justdave

Comment 32

[Bradley Baetz (:bbaetz)]

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