Closed Bug 1253618 Opened 8 years ago Closed 8 years ago

Add support for e10s & asan annotations in Marionette manifests

Categories

(Remote Protocol :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(e10s+, firefox46 fixed, firefox47 fixed, firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: RyanVM, Assigned: impossibus)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

mochitests have valid e10s and asan values for use in creating rules (i.e. |skip-if = e10s| or |skip-if = asan|). They do not appear to work for Marionette. Fixing that would allow us to turn on Mn on ASAN builds (one test that depends on the crash reporter needs skipping) and would allow for a less heavy-handed test skipping approach in bug 1239552 until the underlying problem can be investigated and fixed.
Blocks: e10s-tests
tracking-e10s: --- → +
Re e10s - as per [1], Marionette harness has a skip_if_e10s decorator; which I just tested locally with [2] and it works. You should use that to skip individual tests. 

As for skipping entire files with |skip-if = e10s| or |skip-if = asan|: right now the Mn runner calls manifestparser [3], but only provides values from |mozinfo.info|, so nothing regarding e10s or asan. I'll look into adding the missing data.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1239552#c3
[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/unit/test_about_pages.py#58
[3] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#985-990
Assignee: nobody → mjzffr
Here's a try run that shows |skip-if = e10s| working: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dcad126a2d3&filter-tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false

It seems Mn tests don't ever run against asan builds, but based on code in mozrunner, I understand that 'asan' is included in mozinfo.info, so |skip-if = asan| should already work with Marionette runner. I haven't had a chance to test this myself. Will do so later.
(In reply to Maja Frydrychowicz (:maja_zf) from comment #2)
> It seems Mn tests don't ever run against asan builds, but based on code in
> mozrunner, I understand that 'asan' is included in mozinfo.info, so |skip-if
> = asan| should already work with Marionette runner. I haven't had a chance
> to test this myself. Will do so later.

I filed this because my earlier results say it isn't :). Here's a link to a recent Cedar run showing it:
https://treeherder.mozilla.org/#/jobs?repo=cedar&revision=84b2eeea8164&filter-searchStr=asan%20mn
Attachment #8728167 - Flags: review?(dburns) → review+
Comment on attachment 8728167 [details]
MozReview Request: Bug 1253618 - Add support for |skip-if = e10s| manifest annotations in Marionette runner; r=AutomatedTester

https://reviewboard.mozilla.org/r/38825/#review35547
I've learned that mochitest updates mozinfo with the content of mozinfo.json, which is found in mochitest.tests.zip; then mozinfo.info will contain an 'asan' key. So we need to do the same in Marionette runner, which means I need to get mozinfo.json into common.tests.zip.

Patch forthcoming + I'll file a bug about getting mozinfo.json into common.tests.zip
Comment on attachment 8728167 [details]
MozReview Request: Bug 1253618 - Add support for |skip-if = e10s| manifest annotations in Marionette runner; r=AutomatedTester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38825/diff/1-2/
Attachment #8728167 - Attachment description: MozReview Request: Bug 1253618 - Add support for |skip-if = e10s| manifest annotations in Marionette runner; r?AutomatedTester → MozReview Request: Bug 1253618 - Add support for |skip-if = e10s| manifest annotations in Marionette runner; r=AutomatedTester
Tentative patch up for feedback; |skip-if = asan| works locally in the presence
of a mozinfo.json file. (Skip is ignored if mozinfo.json file is absent)

`update_mozinfo` is copied from mochitest, which I'm not too happy with.
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#553-565

I'm looking into moving this function into some common area, like mozinfo itself.
Any thoughts?

Review commit: https://reviewboard.mozilla.org/r/38999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38999/
Attachment #8728640 - Flags: review?(dburns)
Attachment #8728640 - Flags: review?(dburns)
Comment on attachment 8728640 [details]
MozReview Request: Bug 1253618 - Support |skip-if = asan| manifest annotations in Marionette runner; r?AutomatedTester

https://reviewboard.mozilla.org/r/38999/#review35793

::: testing/marionette/harness/marionette/runner/base.py:35
(Diff revision 1)
> +def update_mozinfo(path=None):

Yea, this looks like it would benefit from being in a more central place. We can do that in a separate bug if you want so that you can update the other runners at the same time.
https://reviewboard.mozilla.org/r/38999/#review35793

> Yea, this looks like it would benefit from being in a more central place. We can do that in a separate bug if you want so that you can update the other runners at the same time.

I filed Bug 1255815 to take care of the moving later.
Here's a try run with extra log messages showing that an "asan" key is getting added to |mozinfo.info| - https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fb55935bf27&selectedJob=18012034
Comment on attachment 8728167 [details]
MozReview Request: Bug 1253618 - Add support for |skip-if = e10s| manifest annotations in Marionette runner; r=AutomatedTester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38825/diff/2-3/
Comment on attachment 8728640 [details]
MozReview Request: Bug 1253618 - Support |skip-if = asan| manifest annotations in Marionette runner; r?AutomatedTester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38999/diff/1-2/
Attachment #8728640 - Flags: review?(dburns)
Attachment #8728640 - Flags: review?(dburns) → review+
Comment on attachment 8728640 [details]
MozReview Request: Bug 1253618 - Support |skip-if = asan| manifest annotations in Marionette runner; r?AutomatedTester

https://reviewboard.mozilla.org/r/38999/#review36559
Seeing failures on some platforms.
gecko.log shows:
>>1458055951320	Marionette	DEBUG	Got request: executeScript, data: {"name":"executeScript","parameters":{"scriptTimeout":null,"newSandbox":true,"args":[],"filename":"base.py","script":"\n            try {\n              return Services.appinfo;\n            } catch (e) {\n              return null;\n            }","sandbox":"default","line":677}}, id: null
>>1458055951331	Marionette	INFO	sendToClient: {"from":"0","error":{"message":"NS_ERROR_FILE_INVALID_PATH: Component returned failure code: 0x80520009 (NS_ERROR_FILE_INVALID_PATH) [nsIFile.target]","status":17,"stacktrace":"execute_script @base.py, line 677\ninline javascript, line 21\nsrc: \"undefined\""}}, {643000ef-03f7-4371-aeda-d8209fb6890e}, {643000ef-03f7-4371-aeda-d8209fb6890e}
I'm inclined to simply work around this error in Marionette runner.

A few questions
- Given that the luciddream tests use b2g, shouldn't they be tier-3 on Treeherder?
- If not, can you provide or prepare a b2g binary that will work with Marionette's call to Services.appinfo?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(fabrice)
luciddream might just be disabled. Let me ask around.
I do not see the direct relation between "NS_ERROR_FILE_INVALID_PATH [nsIFile.target]" and Services.appinfo but yes, you can disable luciddream. I thought it was disable as other b2g test suites.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(fabrice)
Landed these again, since Ld is Tier-3
https://hg.mozilla.org/mozilla-central/rev/38b6ef96c7d0
https://hg.mozilla.org/mozilla-central/rev/b3fa8850897f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.