Vicamo Yang [:vicamo][:vyang] Assignee
	Description • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 1 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 2 • 11 years ago

Add permission 'cellbroadcast' to test-container app.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 3 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 4 • 11 years ago

(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

	Andrew Halberstadt [:ahal]
	Comment 6 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 7 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 9 • 11 years ago

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

Thank you :)

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 10 • 11 years ago

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

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 12 • 11 years ago

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?

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 13 • 11 years ago

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

	Jonathan Griffin (:jgriffin)
	Comment 14 • 11 years ago

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.

	Jonathan Griffin (:jgriffin)
	Comment 15 • 11 years ago

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?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 16 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 17 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 18 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 19 • 11 years ago

fix comment 10, comment 11

	Jonathan Griffin (:jgriffin)
	Comment 21 • 11 years ago

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.

	Jonathan Griffin (:jgriffin)
	Comment 22 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 23 • 11 years ago

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|

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 24 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 25 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=75aa585c7511

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 26 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 27 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 28 • 11 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 29 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 36 • 11 years ago

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
    ];

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 38 • 11 years ago

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

	Ken Chang[:kenkai]
	Comment 39 • 10 years ago

Put this bug into backlog.

	Edgar Chen [:edgar]
	Comment 40 • 10 years ago

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.

	Edgar Chen [:edgar]
	Comment 41 • 10 years ago

(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. :(

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 42 • 10 years ago

(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)

	BMO Automation
	Comment 43 • 7 years ago

Firefox OS is not being worked on