1114695 - Factor most of MarionetteJSTestCase.runTest out into a base class method

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[(not currently active) Ted Mielczarek]

In Luciddream, we want to be able to write Python tests that can also execute JS scripts that function as tests. In general Alex tells me that writing Marionette Python tests that do non-trivial async JavaScript is a pain, so this might be more generally useful.

I copy/pasted most of MarionetteJSTestCase.runTest into luciddream and it works pretty well:
https://github.com/luser/luciddream/blob/c6412d90186f32f705ed23af4d7d8a6b5a25170b/luciddream/__init__.py#L17

I'd like to factor this out into CommonTestCase to avoid the code duplication.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: factor a lot of MarionetteJSTestCase.runTest out into CommonTestCase.run_js_test

This suits my purposes and doesn't seem to make any of the Marionette unit tests fail. There's one thing I wasn't sure about in this patch. MarionetteJSTestCase had a check for "test_" in the filename to look for head files to append. I dropped that but I wasn't sure why it was there in the first place. Any idea? Does it matter?

I'll push this to try at some point.

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8540857 [details] [diff] [review]
factor a lot of MarionetteJSTestCase.runTest out into CommonTestCase.run_js_test

Review of attachment 8540857 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette/marionette_test.py
@@ +500,5 @@
> +        args = []
> +
> +        #XXX: the original code had
> +        # if os.path.basename(self.jsFile).startswith('test_'):
> +        # but I don't know exactly why or if that's critical.

It's not.  It's an artifact of some now-removed support for browser chrome tests, which started with 'browser_' and were handled a bit differently.

@@ +669,5 @@
>  
>      def runTest(self):
>          if self.marionette.session is None:
>              self.marionette.start_session()
>          self.marionette.test_name = os.path.basename(self.jsFile)

We should probably move setting marionette.test_name to run_js_test; it's used by Marionette to return stack traces of JS errors.  With the code we have here, if a JS error occurred in the unit test test_run_js_test.py, you'd get back a stack trace which would point to a line number in test_run_js_test.py, instead of in the JS file being executed, which would be wrong and confusing.

Comment 3

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf22f1909e1f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf22f1909e1f