1428740 - Improve Marionette unit tests for crashes

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Due to bug and deps we had to stop all crash tests from running. I'm working towards re-enabling them. As such I want to see some improvements to the tests first given that I can reproduce the underlying hang during shutdown of Firefox now.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1428740 - Improve Marionette unit tests for parent and content crashes.

Review commit: https://reviewboard.mozilla.org/r/210920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210920/

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8940672 [details]
Bug 1428740 - Improve Marionette unit tests for parent and content crashes.

https://reviewboard.mozilla.org/r/210920/#review217198

Just a question to understand the motivation behind the changes. Perhaps you can explain briefly in the commit message. Thanks.

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py
(Diff revision 1)
>  class TestCrash(BaseCrashTestCase):
>  
>      def test_crash_chrome_process(self):
> -        with self.assertRaises(IOError):
> -            self.crash(chrome=True)
> +        self.assertRaisesRegexp(IOError, "Process crashed",
> +                                self.crash, chrome=True)
> -            Wait(self.marionette, timeout=self.socket_timeout,

Are you sure it's okay to remove the Wait here and elsewhere? Why is it not necessary anymore? Presumably there can still be a delay before the error is raised...

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8940672 [details]
Bug 1428740 - Improve Marionette unit tests for parent and content crashes.

https://reviewboard.mozilla.org/r/210920/#review217198

Ups, I forgot something. I will do.

> Are you sure it's okay to remove the Wait here and elsewhere? Why is it not necessary anymore? Presumably there can still be a delay before the error is raised...

This wait should have only added for content crashes but not a parent (chrome) crash, which immediately kills the process. Also the failure was incorrect. Not sure why this landed...

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8940672 [details]
Bug 1428740 - Improve Marionette unit tests for parent and content crashes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210920/diff/1-2/

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8940672 [details]
Bug 1428740 - Improve Marionette unit tests for parent and content crashes.

https://reviewboard.mozilla.org/r/210920/#review217304

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59bb34f20076
Improve Marionette unit tests for parent and content crashes. r=maja_zf

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/59bb34f20076