Closed
Bug 1478094
Opened 6 years ago
Closed 6 years ago
Update crash unit tests to make use of about:crashparent and about:crashcontent
Categories
(Remote Protocol :: Marionette, enhancement, P3)
Remote Protocol
Marionette
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: whimboo, Assigned: vpit3833, Mentored)
References
Details
(Whiteboard: [lang=py])
User Story
To get started with test automation and the Marionette framework please consult the following documentation: https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html
Attachments
(1 file)
Bug 1462702 brought us two hidden about pages which allow us to crash Firefox. We should update our crash unit tests of Marionette for using the about:crashparent and about:crashcontent pages.
Reporter | ||
Comment 1•6 years ago
|
||
The test file which needs to be changed here is:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py
And here specifically the `crash()` method of the `BaseCrashTestCase` class. Instead of using `execute_script` with some ctypes code, we can simply use `navigate()` with the above mentioned about pages. The navigation always has to happen in the content context, but depending on the `chrome` argument (which I would rename to `parent`) the appropriate about page has to be loaded.
Mentor: hskupin
User Story: (updated)
Priority: P2 → P3
Whiteboard: [lang=py]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8995182 -
Flags: review?(hskupin)
Reporter | ||
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review266736
Thank you for the patch. Please see my opened issues for items which need to be addressed. Also ensure to run the test file.
::: commit-message-7ba07:1
(Diff revision 1)
> +Bug 1478094 - Update crash unit tests to make use of about:crashparent and about:crashcontent
Please add `[marionette]` after the dash so it's better visible for which component this patch is.
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:89
(Diff revision 1)
>
> super(BaseCrashTestCase, self).tearDown()
>
> def crash(self, chrome=True):
> context = 'chrome' if chrome else 'content'
> sandbox = None if chrome else 'system'
This is not needed anymore. Please remove all dead code.
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:94
(Diff revision 1)
> sandbox = None if chrome else 'system'
>
> socket_timeout = self.marionette.client.socket_timeout
> self.marionette.client.socket_timeout = self.socket_timeout
>
> self.marionette.set_context(context)
Please note that with navigation we always have to execute it in content context (as mentioned in my previous comment). As you still set the context here, the tests all fail for chrome context. Did you actually run the test_crash.py file?
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:96
(Diff revision 1)
> socket_timeout = self.marionette.client.socket_timeout
> self.marionette.client.socket_timeout = self.socket_timeout
>
> self.marionette.set_context(context)
> try:
> - self.marionette.execute_script("""
> + self.marionette.navigate("about:crashparent" if chrome else "about:crashcontent")
I would prefer something like:
> self.marionette.navigate("about:crash{}".format(process))
Whereby process is "parent" or "content" based on the function argument, which I also would like to see renamed from `chrome` to `parent`.
Attachment #8995182 -
Flags: review?(hskupin) → review-
Reporter | ||
Updated•6 years ago
|
Assignee: hskupin → venkateshpitta
Assignee | ||
Comment 4•6 years ago
|
||
I submitted for review too soon. Tests fail, and from my understanding of the fail/error messages, all the users of the crash method expect a return value that is not in (None, 0), as well as an IOError be raised with a message as expected by the user. I know this is not possible, at least with Python, and seem stuck here without knowing how to move forward.
Please provide some pointers. Is the script passed to execute_script truly returning something not in (None, 0) as well as raising IOError? How to get this done with Python?
Flags: needinfo?(hskupin)
Reporter | ||
Comment 5•6 years ago
|
||
Can you please give some more details of the failures? I cannot help if nothing is provided. Please show the log entries from one of those tests which fail for you. Note that when an application crashes its exit code should never be 0, if it's None it would mean the process is still alive.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review266736
> Please note that with navigation we always have to execute it in content context (as mentioned in my previous comment). As you still set the context here, the tests all fail for chrome context. Did you actually run the test_crash.py file?
This patch passes the tests.
Reporter | ||
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review267700
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:88
(Diff revision 2)
> self.marionette.crashed = self.crash_count
>
> super(BaseCrashTestCase, self).tearDown()
>
> def crash(self, chrome=True):
> - context = 'chrome' if chrome else 'content'
> + process = 'parent' if chrome else 'content'
Please rename the argument of the method to directly accept `parent` as bool.
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:93
(Diff revision 2)
> - sandbox = None if chrome else 'system'
>
> socket_timeout = self.marionette.client.socket_timeout
> self.marionette.client.socket_timeout = self.socket_timeout
>
> - self.marionette.set_context(context)
> + self.marionette.using_context(process)
This should not work and fail. First `using_context` is a context manager and has to be used with `with`. You can check other tests in this folder for an example.
Second a navigation is only allowed for `content` context, and for nothing else. Also `parent` is not a valid context.
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:95
(Diff revision 2)
> socket_timeout = self.marionette.client.socket_timeout
> self.marionette.client.socket_timeout = self.socket_timeout
>
> - self.marionette.set_context(context)
> + self.marionette.using_context(process)
> try:
> - self.marionette.execute_script("""
> + self.marionette.navigate("about:crash{}".format(process))
This is the right thing to do.
Attachment #8995182 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review267700
> This should not work and fail. First `using_context` is a context manager and has to be used with `with`. You can check other tests in this folder for an example.
>
> Second a navigation is only allowed for `content` context, and for nothing else. Also `parent` is not a valid context.
Does not fail on my PC. Using 'with' causes error (on my PC, as well).
For example,
try:
with self.marionette.using_context("content"):
self.marionette.navigate("about:crash{}".format(process))
finally:
self.marionette.client.socket_timeout = socket_timeout
causes error in these three cases. What am I missing?
Unexpected Results
------------------
ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrash.test_crash_chrome_process - MarionetteException: Please start a session
Traceback (most recent call last):
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
testMethod()
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 105, in test_crash_chrome_process
self.crash, parent=True)
File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
callable_obj(*args, **kwargs)
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
self.marionette.navigate("about:crash{}".format(process))
File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
self.gen.throw(type, value, traceback)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
self.set_context(scope)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
{"value": context})
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
return func(*args, **kwargs)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
raise errors.MarionetteException("Please start a session")
ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInSetUp.test_crash_in_setup - MarionetteException: Please start a session
Traceback (most recent call last):
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 140, in run
self.setUp()
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 158, in setUp
self.crash, parent=True)
File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
callable_obj(*args, **kwargs)
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
self.marionette.navigate("about:crash{}".format(process))
File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
self.gen.throw(type, value, traceback)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
self.set_context(scope)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
{"value": context})
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
return func(*args, **kwargs)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
raise errors.MarionetteException("Please start a session")
ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInTearDown.test_crash_in_teardown - MarionetteException: Please start a session
Traceback (most recent call last):
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 190, in run
self.tearDown()
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 176, in tearDown
self.crash, parent=True)
File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
callable_obj(*args, **kwargs)
File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
self.marionette.navigate("about:crash{}".format(process))
File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
self.gen.throw(type, value, traceback)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
self.set_context(scope)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
{"value": context})
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
return func(*args, **kwargs)
File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
raise errors.MarionetteException("Please start a session")
Reporter | ||
Comment 10•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review267700
> Does not fail on my PC. Using 'with' causes error (on my PC, as well).
>
> For example,
>
> try:
> with self.marionette.using_context("content"):
> self.marionette.navigate("about:crash{}".format(process))
> finally:
> self.marionette.client.socket_timeout = socket_timeout
>
> causes error in these three cases. What am I missing?
>
> Unexpected Results
> ------------------
> ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrash.test_crash_chrome_process - MarionetteException: Please start a session
> Traceback (most recent call last):
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
> testMethod()
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 105, in test_crash_chrome_process
> self.crash, parent=True)
> File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
> callable_obj(*args, **kwargs)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
> self.marionette.navigate("about:crash{}".format(process))
> File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
> self.gen.throw(type, value, traceback)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
> self.set_context(scope)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
> {"value": context})
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
> return func(*args, **kwargs)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
> raise errors.MarionetteException("Please start a session")
> ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInSetUp.test_crash_in_setup - MarionetteException: Please start a session
> Traceback (most recent call last):
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 140, in run
> self.setUp()
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 158, in setUp
> self.crash, parent=True)
> File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
> callable_obj(*args, **kwargs)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
> self.marionette.navigate("about:crash{}".format(process))
> File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
> self.gen.throw(type, value, traceback)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
> self.set_context(scope)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
> {"value": context})
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
> return func(*args, **kwargs)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
> raise errors.MarionetteException("Please start a session")
> ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInTearDown.test_crash_in_teardown - MarionetteException: Please start a session
> Traceback (most recent call last):
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 190, in run
> self.tearDown()
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 176, in tearDown
> self.crash, parent=True)
> File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
> callable_obj(*args, **kwargs)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
> self.marionette.navigate("about:crash{}".format(process))
> File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
> self.gen.throw(type, value, traceback)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
> self.set_context(scope)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
> {"value": context})
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
> return func(*args, **kwargs)
> File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
> raise errors.MarionetteException("Please start a session")
Oh, that reminds me to a problem we still have and that's why it was changed from `using_context()` to `set_context()`. The reason why it's failing is that the context manager (with) will try to reset the context when its leaving the code block. But at this time the crash happened and there is no active session anymore.
So please don't use `using_context()` but leave it as `set_context()`. Then it will be fine.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review267700
> Oh, that reminds me to a problem we still have and that's why it was changed from `using_context()` to `set_context()`. The reason why it's failing is that the context manager (with) will try to reset the context when its leaving the code block. But at this time the crash happened and there is no active session anymore.
>
> So please don't use `using_context()` but leave it as `set_context()`. Then it will be fine.
Good to know. I just submitted a review. Shall I add a note in the crash method explaining this to the future?
Reporter | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review267700
> Good to know. I just submitted a review. Shall I add a note in the crash method explaining this to the future?
No, you can leave it out. Hopefully not that far in the future I will be able to finish bug 1433873 which will fix it.
Reporter | ||
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review268014
This looks good to me. Lets just fix the one minor nit.
Meanwhile I will push a try build to verify everything is working as expected.
Thanks!
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:88
(Diff revision 3)
> self.marionette.crashed = self.crash_count
>
> super(BaseCrashTestCase, self).tearDown()
>
> - def crash(self, chrome=True):
> - context = 'chrome' if chrome else 'content'
> + def crash(self, parent=True):
> + process = 'parent' if parent else 'content'
Lets get rid of this extra variable and directly use it in the `format()` call.
Attachment #8995182 -
Flags: review?(hskupin) → review+
Comment hidden (mozreview-request) |
Reporter | ||
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review268190
::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:94
(Diff revisions 3 - 4)
> self.marionette.client.socket_timeout = self.socket_timeout
>
> self.marionette.set_context("content")
> try:
> - self.marionette.navigate("about:crash{}".format(process))
> + self.marionette.navigate("about:crash{}".
> + format("parent" if parent else "content"))
I think the linter might not like it. Can you better format it like the following?
> self.marionette.navigate(
> "about:crash{}".format("parent" if parent else "content"))
This will also be much better readable.
Thanks.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent
https://reviewboard.mozilla.org/r/259674/#review268190
> I think the linter might not like it. Can you better format it like the following?
>
> > self.marionette.navigate(
> > "about:crash{}".format("parent" if parent else "content"))
>
> This will also be much better readable.
>
> Thanks.
Oh, looked different on BugZilla. I broke the line because it is longer than 80 characters. I am pasting this code shown by you verbatim and submitting new review.
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/864df2345f9b
[marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent r=whimboo
Comment 21•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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
•