Closed Bug 979454 Opened 6 years ago Closed 6 years ago

manifest parser needs to support skip-if in the [default] section and || that with the skip-if from the [test] section

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: jmaher, Assigned: jmaher)

Details

Attachments

(1 file, 1 obsolete file)

currently manifestparser supports skip-if in the [default] section, but if the [test] has a skip-if, that skip-if overwrites the value in the global.  This is problematic for us when converting android|b2g.json manifests to ini.
Attached patch manifest_defaults.patch (obsolete) — Splinter Review
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #8385491 - Flags: review?
Attachment #8385491 - Flags: review? → review?(ted)
Comment on attachment 8385491 [details] [diff] [review]
manifest_defaults.patch

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

Thanks for writing a test!

::: testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
@@ +390,4 @@
>      # interpret the variables
>      def interpret_variables(global_dict, local_dict):
>          variables = global_dict.copy()
> +        if local_dict.get('skip-if', '') and variables.get('skip-if', ''):

I would say "if 'skip-if' in local_dict and 'skip-if' in variables:"

@@ +395,2 @@
>          variables.update(local_dict)
> +            

I think you have goofy whitespace here.
Attachment #8385491 - Flags: review?(ted) → review+
sorry for the rereview, I have had to do some more logic to remove the comments. With this patch, we are good on try server.
Attachment #8385491 - Attachment is obsolete: true
Attachment #8385996 - Flags: review?
Attachment #8385996 - Flags: review? → review?(ted)
Comment on attachment 8385996 [details] [diff] [review]
preserve default skip-if when test has a skip-if also (1.1)

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

::: testing/mozbase/manifestdestiny/manifestparser/manifestparser.py
@@ +390,5 @@
>      # interpret the variables
>      def interpret_variables(global_dict, local_dict):
>          variables = global_dict.copy()
> +        if 'skip-if' in local_dict and 'skip-if' in variables:
> +            local_dict['skip-if'] = "(%s) || (%s)" % (variables['skip-if'].split('#')[0], local_dict['skip-if'].split('#')[0])

Add a test for this behavior? We should probably make this less awful somehow.
Attachment #8385996 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/7373f3e5d57a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.