Extensions templates are not tested by the normal sanity test scripts

RESOLVED FIXED in Bugzilla 4.2

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

unspecified
Bugzilla 4.2
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

8 years ago
Created attachment 538913 [details] [diff] [review]
Patch to enabled testing of extension templates (v1)
Assignee: extensions → dkl
Status: NEW → ASSIGNED
Attachment #538913 - Flags: review?(mkanat)

Comment 2

8 years ago
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.)
Attachment #538913 - Flags: review?(mkanat) → review-
(Assignee)

Comment 3

8 years ago
Created attachment 539326 [details] [diff] [review]
Patch to enabled testing of extension templates (v1)

New patch with suggested changes.

dkl
Attachment #538913 - Attachment is obsolete: true
Attachment #539326 - Flags: review?(mkanat)
(Assignee)

Comment 4

8 years ago
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

8 years ago
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.)
Attachment #539326 - Flags: review?(mkanat) → review-
(Assignee)

Comment 6

8 years ago
Created attachment 541144 [details] [diff] [review]
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
Attachment #539326 - Attachment is obsolete: true
Attachment #541144 - Flags: review?(mkanat)

Comment 7

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

Frickin beautiful. :-)
Attachment #541144 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval+
Target Milestone: --- → Bugzilla 4.2
(Assignee)

Comment 8

8 years ago
trunk:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified t/Support/Templates.pm
modified t/004template.t
Committed revision 7842. 

dkl
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.