Closed Bug 1478094 Opened Last year Closed Last year

Update crash unit tests to make use of about:crashparent and about:crashcontent

Categories

(Testing :: Marionette, enhancement, P3)

enhancement

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.
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]
Attachment #8995182 - Flags: review?(hskupin)
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-
Assignee: hskupin → venkateshpitta
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)
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 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.
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-
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")
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 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?
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.
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 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 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.
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
https://hg.mozilla.org/mozilla-central/rev/864df2345f9b
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.