663835 - Extensions templates are not tested by the normal sanity test scripts

Status: RESOLVED FIXED

(Bugzilla :: Extensions, defect)

Description

[David Lawrence [:dkl]]

When certain sanity scripts are ran by runtests.pl such as 004template.t and 008filter.t, the templates in extensions/*/template are not tested.

Patch coming that adds enabled extensions templates to the list to test.

dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to enabled testing of extension templates (v1)

Comment 2

[Max Kanat-Alexander]

Comment on attachment 538913 [details] [diff] [review]
Patch to enabled testing of extension templates (v1)

Review of attachment 538913 [details] [diff] [review]:
-----------------------------------------------------------------

This is awesome! I think there's one cleanup we should do, though:

::: t/Support/Templates.pm
@@ +75,5 @@
> +        foreach my $extension (@extensions) {
> +            $path = File::Spec->catdir($extension . '/template', $langdir, 'custom');
> +            $dirs{$path} = 1 if -d $path;
> +            $path = File::Spec->catdir($extension . '/template', $langdir, 'default');
> +            $dirs{$path} = 1 if -d $path;

This should be using template_include_path instead of all of this custom code. (I know the previous code wasn't, but if we're going to be checking every template, this is a good time to start.)

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to enabled testing of extension templates (v1)

New patch with suggested changes.

dkl

Comment 4

[David Lawrence [:dkl]]

Just a note that when this patch lands and any of the included extensions (Voting, OldBugMove, Example, etc.) are enabled on an installation, runtests.pl will throw errors that were not there before. I will open new bugs for each of those to fix separately. For example t/008filter.t will throw errors complaining that a filterexceptions.pl file does not exist for each of the extensions template dirs. Adding a clean filterexceptions.pl file to each then throws more errors about all of the vars in the templates that are not filtered.

dkl

Comment 5

[Max Kanat-Alexander]

Comment on attachment 539326 [details] [diff] [review]
Patch to enabled testing of extension templates (v1)

Review of attachment 539326 [details] [diff] [review]:
-----------------------------------------------------------------

::: t/Support/Templates.pm
@@ +62,5 @@
>  # Scan for the template available languages and include paths
>  {
> +    foreach my $langdir (Bugzilla->languages) {
> +        my %dirs;
> +        my $paths = template_include_path({ language => [ $langdir ] });

I wonder, would it just be simpler to say:

  @include_paths = @{ template_include_path({ language => Bugzilla->languages }) };

That would also guarantee a consistent order (which keys %dirs doesn't).

Then you could just do @languages = @{ Bugzilla->languages } for @languages.

@@ +64,5 @@
> +    foreach my $langdir (Bugzilla->languages) {
> +        my %dirs;
> +        my $paths = template_include_path({ language => [ $langdir ] });
> +        foreach my $path (@$paths) {
> +            $dirs{$path} = 1 if -d $path;

template_include_path already checks that for you; everything it returns is a valid directory. (See _template_lang_directories in Install::Util.)

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to enabled testing of extension templates (v2)

Thanks for the review. Here is a simplified patch that uses your suggestions. I had to also update t/004template.t to use @include_paths instead of the $include_path{$lang} colon delimited strings. Works well for me.

dkl

Comment 7

[Max Kanat-Alexander]

Comment on attachment 541144 [details] [diff] [review]
Patch to enabled testing of extension templates (v2)

Frickin beautiful. :-)

Comment 8

[David Lawrence [:dkl]]

trunk:
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified t/Support/Templates.pm
modified t/004template.t
Committed revision 7842. 

dkl