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

RESOLVED FIXED in mozilla38

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

({pi-marionette-runner})

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
Attachment #8540857 - Flags: review?(jgriffin)
Assignee: nobody → ted
Status: NEW → ASSIGNED
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.
Attachment #8540857 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/cf22f1909e1f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.