Currently we have almost all RIL test cases based on xpcshell or Marionette and they're always running in chrome process. Sometimes we do have trouble verifying works related to OOP code. Everything just has to go manually and that means more risk and incompleteness -- bug 907585 is a good example. We also suffer from function breakages, bug 912517 and bug 912861.

Blocked by bug 926280. We'd either have same Marionette script running under both modes or rewrite everything to have a Mochitest version. Surely the former is more preferable for now.

Add permission 'cellbroadcast' to test-container app.

This patch is based on Jonathan's attachment 828181 [details] [diff] [review]. Actual behaviours: 1) ./mach marionette-webapi <dir>/ - selects all test cases under <dir> and execute them only in in-process mode. 2) ./mach marionette-webapi --type=+oop <dir>/ - selects all test cases under <dir> and execute them only in oop mode. 3) ./mach marionette-webapi --type=-oop <dir>/ - selects all test cases under <dir> and execute them only in in-process mode. 4) ./mach marionette-webapi <dir>/manifest.ini - selects all active test cases listed in <dir>/manifest.ini and execute them in the mode(s) according to the manifest. 5) ./mach marionette-webapi --type=+oop <dir>/manifest.ini - selects all active, oop={true,both} test cases listed in <dir>/manifest.ini and execute them only in oop mode. 6) ./mach marionette-webapi --type=-oop <dir>/manifest.ini - selects all active, oop={false,both} test cases listed in <dir>/manifest.ini and execute them only in in-process mode.

(In reply to Vicamo Yang [:vicamo][:vyang] (PTO Dec. 21 ~ Jan. 5) from comment #3) > Created attachment 8349384 [details] [diff] [review] > part 1/3: refine Marionette oop test case selection try: https://tbpl.mozilla.org/?tree=Try&rev=3f4a33f8cf18

Comment on attachment 8349384 [details] [diff] [review] part 1/3: refine Marionette oop test case selection I'd feel more comfortable if jgriffin or mdas reviewed this one.

1) fix oop mode selection for single test file as well 2) fix |self.tests| growing problem when running with --repeat Will request for review once the two oranges in previous try are solved.

Fix Mn orange. Try: https://tbpl.mozilla.org/?tree=Try&rev=d962aa689417

(In reply to Andrew Halberstadt [:ahal] from comment #6) > I'd feel more comfortable if jgriffin or mdas reviewed this one. Thank you :)

Failed to receive "window.onapploadtime" event after a few successful OOP test runs. Hmm...

Comment on attachment 8349444 [details] [diff] [review] part 1/3: refine Marionette oop test case selection : v3 Review of attachment 8349444 [details] [diff] [review]: ----------------------------------------------------------------- Just a few questions before I r+, and can you run this through try to make sure the Mn and Gu tests pass? ::: testing/marionette/client/marionette/runner/base.py @@ -785,5 @@ > (filename.endswith('.py') or filename.endswith('.js'))): > filepath = os.path.join(root, filename) > - self.run_test(filepath) > - if self.marionette.check_for_crash(): > - return why is the check_for_crash removed in the new patch? @@ +784,3 @@ > return > > mod_name,file_ext = os.path.splitext(os.path.split(filepath)[-1]) I think this can be removed since it's only used in run_test_set @@ +803,5 @@ > self.appName)) > self.todo += 1 > > target_tests = manifest.get(tests=manifest_tests, **testargs) > + if testarg_oop is not None: shouldn't this be "if testarg_oop"? I don't think we want to set 'both' if someone passes -oop to --type. -oop sets oop to False, where as +oop sets it to True @@ +805,5 @@ > > target_tests = manifest.get(tests=manifest_tests, **testargs) > + if testarg_oop is not None: > + testargs['oop'] = 'both' > + target_tests += manifest.get(tests=manifest_tests, **testargs); extra semi-colon at the end of the line @@ +813,5 @@ > + manifest_oop = i.get('oop') > + if testarg_oop != 'false' and (manifest_oop == 'both' or manifest_oop == 'true'): > + self.add_test(i["path"], i["expected"], True) > + if testarg_oop != 'true' and (manifest_oop == 'both' or manifest_oop != 'true'): > + self.add_test(i["path"], i["expected"], False) The logic in the above two if statements is clever but hard to understand for someone who isn't familiar with why this is here. It would be helpful to add a comment above these lines, saying something like "The following lines let you run the same test with oop and without oop if either of the following conditions are met: when testarg_oop is 'both' and 1) manifest_oop is 'true' or 2) manifest_oop is 'both'. If these conditions are not met, it will run the test as expected, ie: the test will run with oop only if testarg_oop is 'true' and the manifest_oop is 'true' or 'both'" @@ +866,5 @@ > + oop_tests = [x for x in self.tests if x.get('oop')] > + self.run_test_set(oop_tests) > + > + in_process_tests = [x for x in self.tests if not x.get('oop')] > + self.run_test_set(in_process_tests) Why must the oop tests run first? Can they run along side non-oop tests?

Ah, I see this passes Mn, but Gu would be good to test as well.

Comment on attachment 8349444 [details] [diff] [review] part 1/3: refine Marionette oop test case selection : v3 Review of attachment 8349444 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/marionette/client/marionette/runner/base.py @@ +866,5 @@ > + oop_tests = [x for x in self.tests if x.get('oop')] > + self.run_test_set(oop_tests) > + > + in_process_tests = [x for x in self.tests if not x.get('oop')] > + self.run_test_set(in_process_tests) In response to Malini, the OOP tests run first because they run inside the test-container app. When we run in-process tests, we unload Gaia's system app, and we can't easily get back to that state. So, we run the OOP tests first, then the in-process tests.

Comment on attachment 8349444 [details] [diff] [review] part 1/3: refine Marionette oop test case selection : v3 Review of attachment 8349444 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, thanks for doing this. We should also do a try run along with the manifest changes, too. r- only for the testargs['oop'] = 'both' logic, which seems suspect, but I'm willing to be convinced otherwise. ::: testing/marionette/client/marionette/runner/base.py @@ +805,5 @@ > > target_tests = manifest.get(tests=manifest_tests, **testargs) > + if testarg_oop is not None: > + testargs['oop'] = 'both' > + target_tests += manifest.get(tests=manifest_tests, **testargs); I don't understand this logic either; I think it works ok if a manifest defaults to oop = both, but what if it defaulted to oop = false and a few tests set oop = both, or other variations?

1) Update commit summary. 2) Add back check_for_crash(). It's originally at the next line to |run_test| call. Since the test execution part of |run_test| has been moved into |run_test_set|, I place a |check_for_crash()| at the end of tests execution loop of |run_test_set|. 3) > > mod_name,file_ext = os.path.splitext(os.path.split(filepath)[-1]) > I think this can be removed since it's only used in run_test_set 'mod_name' is referenced in |run_test_set| and 'file_ext' is in |add_test| 4) Don't filter tests by "oop" flag because manifest parser can't handle it well. 5) Add test cases for oop test selection in testing/marionette/client/marionette/tests/unit/oop/. However, these test cases are temporarily disabled because of the problem in comment 10 and comment 11. 6) try: https://tbpl.mozilla.org/?tree=Try&rev=10944172441f

(In reply to Vicamo Yang [:vicamo][:vyang] (PTO Dec. 21 ~ Jan. 5) from comment #16) > Created attachment 8349880 [details] [diff] [review] > part 1/3: refine Marionette oop test case selection : v4 Didn't request Malini's review because she takes PTO from today, but your opinions are still welcome and appreciated.

This patch prints debug messages to show which test cases are selected. To verify, just execute: $ ./mach marionette-webapi `pwd`/gecko/testing/marionette/client/marionette/tests/unit/oop/manifest-*.ini $ ./mach marionette-webapi --type=-oop `pwd`/gecko/testing/marionette/client/marionette/tests/unit/oop/manifest-*.ini $ ./mach marionette-webapi --type=+oop `pwd`/gecko/testing/marionette/client/marionette/tests/unit/oop/manifest-*.ini All combinations ['unspecified', 'false', 'true', 'both'] x ['manifest default', 'per test case'] are considered.

fix comment 10, comment 11

Comment on attachment 8349947 [details] [diff] [review] part 2/3: only launch test-container app when not available Review of attachment 8349947 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense, thanks.

Comment on attachment 8349880 [details] [diff] [review] part 1/3: refine Marionette oop test case selection : v4 Review of attachment 8349880 [details] [diff] [review]: ----------------------------------------------------------------- Awesome! Thanks for the extra code comments and the unit tests. ::: testing/marionette/client/marionette/runner/base.py @@ +815,5 @@ > raise IOError("test file: %s does not exist" % i["path"]) > + > + # manifest_oop is either 'false', 'true' or 'both'. Anything > + # else implies 'false'. > + manifest_oop = i.get('oop') This should probably be i.get('oop', 'false'), to match your comments, although it doesn't affect anything now since we only test for manifest_oop == 'true' or 'both'.

1) rebase to HEAD 2) address previous review comment 3) skip oop for .py test cases 4) address comment 12 and comment 16, 'file_ext' is only used in |add_test| and 'mod_name' in |run_test_set|

A minor fix -- only switch back to main frame when is oop.

1) Extend timeouts for some test cases because they take longer time to finish. 2) disable oop for some chrome only test cases 3) investigate fail causes TODO: 1) can't use Promise in oop tests? 2) iccIds.length = 0 3) adjust timeouts according to tryservers, because they're even more slower.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #26) > TODO: > 1) can't use Promise in oop tests? It seems somehow SpecialPowerObserverAPI never processes requests from SpecialPowers in OOP mode, so test cases relying on effectiveness of `pushPermissions`, `pushFooPrefs`, ..., will either fail with timeout or value mismatches.

Depends on bug 952875, which fixes test_incoming.js in oop mode.

Comment on attachment 8349372 [details] Github PR for gaia A new test case "test_mobile_last_known_network.js" was added in bug 952371 and it takes an additional permission -- mobilenetwork.

Comment on attachment 8357583 [details] [diff] [review] part 3.a/3: enable oop on all RIL marionette tests : WIP 3 >diff --git a/dom/mobilemessage/tests/marionette/manifest.ini b/dom/mobilemessage/tests/marionette/manifest.ini >--- a/dom/mobilemessage/tests/marionette/manifest.ini >+++ b/dom/mobilemessage/tests/marionette/manifest.ini > [test_message_classes.js] >+; test-container app closed by sms app. >+oop = false Somehow we have: marionette._send_message BEGIN client.send: { "name": "executeJSScript", ... } client.receive: { "emulator_cmd": ... } client.send: { "name": "emulatorCmdResult", ... } client.receive: { "emulator_cmd": ... } client.send: { "name": "emulatorCmdResult", ... } ... client.receive: { "emulator_cmd": ... } client.send: { "name": "emulatorCmdResult", ... } client.receive: {"from":"0","ok":true} marionette._send_message END Because we're expecting a response with response_key == "value" for a "executeJSScript" request, this results in a MarionetteException. This can be only reproduced when we have both PDU_PID_ANSI_136_R_DATA and PDU_PID_USIM_DATA_DOWNLOAD tested in file test_message_classes.js, function test_message_class_2: function test_message_class_2() { ... let allPIDs = [ ..., PDU_PID_ANSI_136_R_DATA, PDU_PID_USIM_DATA_DOWNLOAD ];

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #36) > Because we're expecting a response with response_key == "value" for a > "executeJSScript" request, this results in a MarionetteException. We emits system message "sms-received" all the time during mobilemessage test processes. This brings up Gaia SMS app, triggers a "mozbrowsershowmodalprompt" event, and the |modalHandler| in marionette-listener sends a "MarionetteFrame:handleModal" message to marionette-frame-manager, which then calls |this.sever.sendOk| and results in misinterpreting the response in marionette.py.

Put this bug into backlog.

I ran the OOP tests for all RIL components one by one, only cellbroadcast can pass the OOP tests without any modification. For other components, they probably need to have some fixing/refactoring in order to get pass. So, I would like to file separated bugs for each component (telephony/mobileconnection/mobilemessage/voicemail/cellbroadcast/icc) and mark this bug as meta bug.

(In reply to Edgar Chen [:edgar][:echen] from comment #40) > I ran the OOP tests for all RIL components one by one, only cellbroadcast > can pass the OOP tests without any modification. I can pass cellbroadcast OOP test in my local environment. But it failed in try server, https://tbpl.mozilla.org/?tree=Try&rev=91005fdb6c0d Have no idea why it failed in try yet. :(

(In reply to Edgar Chen [:edgar][:echen] from comment #40) > I ran the OOP tests for all RIL components one by one, only cellbroadcast > can pass the OOP tests without any modification. For other components, they > probably need to have some fixing/refactoring in order to get pass. > > So, I would like to file separated bugs for each component > (telephony/mobileconnection/mobilemessage/voicemail/cellbroadcast/icc) and > mark this bug as meta bug. (y)

Firefox OS is not being worked on