Closed
Bug 1272414
Opened 9 years ago
Closed 9 years ago
Ditch browser/b2g/qemu filtering in marionette manifests
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox49 fixed)
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•9 years 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•9 years ago
|
||
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 3•9 years ago
|
||
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•9 years 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•9 years 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...)
Comment 6•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
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•9 years 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•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
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•9 years ago
|
Attachment #8751828 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8754489 -
Flags: review?(dburns) → review+
Comment 10•9 years ago
|
||
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
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 13•9 years ago
|
||
FTR I also ported/landed the loop affecting part in the Loop repo:
https://github.com/mozilla/loop/commit/6616599dd0edde58bd8e4799a4d84918915334d1
Assignee | ||
Comment 14•9 years 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.
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•