Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from bug 1223277 comment #100)
> Sorry for not getting back to this, but ASAN builds disable the
> crashreporter, so tests for crash reporting should not be run on them.

Right now we disabled all the Marionette unit tests for ASAN builds. With this information we should be able to unskip most of them, except the crash tests.

	Andreas Tolfsen ❲:ato❳
	Comment 2 • 7 years ago

Is running Marionette on ASAN builds, which I understand are quite
slow, good use of resources?  What do we gain from running Mn on
ASAN builds?

A digression is that it seems to me that the way the Mn job is
currently disabled should really have been done at a TC level so
that TC would not have to allocate a test slave in the first place:

> [include:unit/unit-tests.ini]
> skip-if = asan

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

Each crash we can find with Marionette by using ASAN builds is helpful. In all those cases they are most likely security related. I would like to see that enabled because only our harness can do all the restart logic.

	Andreas Tolfsen ❲:ato❳
	Comment 4 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #3)

> Each crash we can find with Marionette by using ASAN builds
> is helpful. In all those cases they are most likely security
> related. I would like to see that enabled because only our harness
> can do all the restart logic.

That sounds reasonable, but I was wondering if there was a
precedence for that.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8920983 [details]
Bug 1410355 - Re-enable Marionette unit tests for ASAN builds except crash tests.

https://reviewboard.mozilla.org/r/191954/#review197248

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #4)
> That sounds reasonable, but I was wondering if there was a
> precedence for that.

We actually disabled the tests via bug 1223277 comment 90 and following to get the click + close window code passing.

(In reply to Andreas Tolfsen ‹:ato› from comment #2)
> A digression is that it seems to me that the way the Mn job is
> currently disabled should really have been done at a TC level so
> that TC would not have to allocate a test slave in the first place:
> 
> > [include:unit/unit-tests.ini]
> > skip-if = asan

We still run the other existing tests of that manifest. So it's all fine.

Lets observe the results in the next months to see how valuable it still is to run the ASAN builds for our unit tests. Since disabled I haven't seen any new bug filed from the other Marionette tests.

	Pulsebot
	Comment 7 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/363bf0516c89
Re-enable Marionette unit tests for ASAN builds except crash tests. r=ato

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 8 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/363bf0516c89

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 9 • 7 years ago

test-only change which might be good to have on beta if it applies cleanly. Otherwise lets wontfix.

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-release/rev/0951afb05f1c