If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ditch browser/b2g/qemu filtering in marionette manifests

RESOLVED FIXED in Firefox 49

Status

Testing
Marionette
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
I could be convinced to make this work only for true|True|TRUE|false|False|FALSE, but this seems like it follows "simplest thing that could possibly work".
(Assignee)

Comment 2

a year ago
Created attachment 8751828 [details]
MozReview Request: Bug 1272414 - 'true' and 'false' literals in manifests should work irrespective of case, r?chmanchester

Review commit: https://reviewboard.mozilla.org/r/52233/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52233/
Attachment #8751828 - Flags: review?(cmanchester)
Comment on attachment 8751828 [details]
MozReview Request: Bug 1272414 - 'true' and 'false' literals in manifests should work irrespective of case, r?chmanchester

https://reviewboard.mozilla.org/r/52233/#review49291

I tried this on top of the issue in bug 888624, and it doesn't address that issue. Marionette is using a part of the manifest parser api that doesn't evaluate values in sections as expressions, it's just comparing strings.

Either way, I think this patch is a little heavy handed. Ideally marionette would use manifest filters like skip-if, evaluate them like other harnesses do, and fail on something like "browser = True" because True is an unknown identifier.
Attachment #8751828 - Flags: review?(cmanchester)
(Assignee)

Comment 4

a year ago
(In reply to Chris Manchester (:chmanchester) from comment #3)
> Comment on attachment 8751828 [details]
> MozReview Request: Bug 1272414 - 'true' and 'false' literals in manifests
> should work irrespective of case, r?chmanchester
> 
> https://reviewboard.mozilla.org/r/52233/#review49291
> 
> I tried this on top of the issue in bug 888624, and it doesn't address that
> issue. Marionette is using a part of the manifest parser api that doesn't
> evaluate values in sections as expressions, it's just comparing strings.
> 
> Either way, I think this patch is a little heavy handed. Ideally marionette
> would use manifest filters like skip-if, evaluate them like other harnesses
> do, and fail on something like "browser = True" because True is an unknown
> identifier.

OK. It seems the skip-ifs are already honoured ( https://dxr.mozilla.org/mozilla-central/search?q=skip-if%20path%3Amanifest.ini&=mozilla-central&redirect=true ).

Could we basically just convert the "b2g" and "browser" and "qemu" stuff to things that evaluate as expressions we could use for skip-if, and then replace the browser/qemu/b2g stuff in the manifest files with skip-ifs/run-ifs? Is that what you're suggesting or were you suggesting something else? Would this be acceptable to the marionette folks?
Flags: needinfo?(dburns)
Flags: needinfo?(cmanchester)
(Assignee)

Comment 5

a year ago
(FWIW, I would personally still be happier if the standard skip-if notation, too, would understand "True" and "TRUE" and so on, but if that's not what other people want, we can keep it as-is...)
Yes, I think that would be reasonable. If we like the current annotations, possibly we could translate them to skip-if expressions and pass them to a manifestparser filter in the harness. However, the supported parameters look useful mostly for enabling/disabling tests on b2g, so a more extensive update might be appropriate.
Flags: needinfo?(cmanchester)
I am ok with doing that. Those are hangovers from Marionette starting out as a B2G only tool and growing. Since we are switching off B2G related things and slowly removing b2g-isms from the runner we should totally do this.
Flags: needinfo?(dburns)
(Assignee)

Updated

a year ago
Component: Mozbase → Marionette
Summary: manifestparser shouldn't treat |true| and |false| case-sensitively → Ditch browser/b2g/qemu filtering in marionette manifests
(Assignee)

Comment 8

a year ago
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2de8567733e4
(Assignee)

Comment 9

a year ago
Created attachment 8754489 [details]
MozReview Request: Bug 1272414 - rip out qemu/browser/b2g flags from marionette manifests, r?AutomatedTester

Review commit: https://reviewboard.mozilla.org/r/53994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53994/
Attachment #8754489 - Flags: review?(dburns)
(Assignee)

Updated

a year ago
Attachment #8751828 - Attachment is obsolete: true
Attachment #8754489 - Flags: review?(dburns) → review+
Comment on attachment 8754489 [details]
MozReview Request: Bug 1272414 - rip out qemu/browser/b2g flags from marionette manifests, r?AutomatedTester

https://reviewboard.mozilla.org/r/53994/#review50768
Blocks: 1274408

Comment 11

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3ccec9b45d

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f3ccec9b45d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
FTR I also ported/landed the loop affecting part in the Loop repo:

https://github.com/mozilla/loop/commit/6616599dd0edde58bd8e4799a4d84918915334d1
(Assignee)

Comment 14

a year ago
(In reply to Mark Banner (:standard8) from comment #13)
> FTR I also ported/landed the loop affecting part in the Loop repo:
> 
> https://github.com/mozilla/loop/commit/
> 6616599dd0edde58bd8e4799a4d84918915334d1

As noted on IRC at the time, if this gets uplifted into repos that don't have this cset, then your tests might not run on infra.
Duplicate of this bug: 873644
Blocks: 1281619
You need to log in before you can comment on or make changes to this bug.