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+
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.