Closed
Bug 1272414
Opened 7 years ago
Closed 7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2de8567733e4
Assignee | ||
Comment 9•7 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•7 years ago
|
Attachment #8751828 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8754489 -
Flags: review?(dburns) → review+
Comment 10•7 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 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f3ccec9b45d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 13•7 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•7 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 months ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•