Closed Bug 926277 Opened 6 years ago Closed 2 years ago

[meta] Add test cases running in OOP mode for RIL APIs

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Whiteboard: [grooming])

Attachments

(11 files, 10 obsolete files)

364 bytes, text/html
ahal
: review+
Details
19.40 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.03 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.75 KB, patch
Details | Diff | Splinter Review
6.52 KB, patch
Details | Diff | Splinter Review
9.54 KB, patch
Details | Diff | Splinter Review
3.40 KB, patch
Details | Diff | Splinter Review
3.20 KB, patch
Details | Diff | Splinter Review
2.01 KB, patch
Details | Diff | Splinter Review
5.39 KB, patch
Details | Diff | Splinter Review
3.84 KB, patch
Details | Diff | Splinter Review
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.
Depends on: 926280
Blocks: 907585
Attached file Github PR for gaia
Add permission 'cellbroadcast' to test-container app.
Assignee: nobody → vyang
Attachment #8349372 - Flags: review?(ahalberstadt)
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.
Attachment #8349384 - Flags: review?(jgriffin)
Attachment #8349384 - Flags: review?(ahalberstadt)
(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
Attachment #8349372 - Flags: review?(ahalberstadt) → review+
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.
Attachment #8349384 - Flags: review?(ahalberstadt)
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.
Attachment #8349384 - Attachment is obsolete: true
Attachment #8349384 - Flags: review?(jgriffin)
Fix Mn orange.  Try: https://tbpl.mozilla.org/?tree=Try&rev=d962aa689417
Attachment #8349411 - Attachment is obsolete: true
Attachment #8349444 - Flags: review?(mdas)
Attachment #8349444 - Flags: review?(jgriffin)
(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...
Attached file missing apploadtime event.log (obsolete) —
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?
Attachment #8349444 - Flags: review?(jgriffin) → review-
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
Attachment #8349444 - Attachment is obsolete: true
Attachment #8349444 - Flags: review?(mdas)
Attachment #8349880 - Flags: review?(jgriffin)
(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.
Attached patch debug-oop-test-selection.patch (obsolete) — Splinter Review
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.
Attachment #8349390 - Attachment is obsolete: true
Attachment #8349466 - Attachment is obsolete: true
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.
Attachment #8349947 - Flags: review?(jgriffin) → review+
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'.
Attachment #8349880 - Flags: review?(jgriffin) → review+
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|
Attachment #8349880 - Attachment is obsolete: true
Attachment #8350602 - Flags: review+
A minor fix -- only switch back to main frame when is oop.
Attachment #8349947 - Attachment is obsolete: true
Attachment #8350604 - Flags: review+
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.
Attachment #8349948 - Attachment is obsolete: true
(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.
Depends on: 952875
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.
Attachment #8349372 - Flags: review+ → review?(ahalberstadt)
Attachment #8357588 - Attachment description: part 3.e/3: 3.e/3: fix test_outgoing_answer_hangup_oncallschanged.js → part 3.e/3: fix test_outgoing_answer_hangup_oncallschanged.js
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
    ];
Attachment #8349372 - Flags: review?(ahalberstadt) → review+
(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.
Depends on: 970804
Put this bug into backlog.
blocking-b2g: --- → backlog
Depends on: 1007062
Depends on: 1075437
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)
Depends on: 1079880
Summary: Add test cases running in OOP mode for RIL APIs → [meta] Add test cases running in OOP mode for RIL APIs
Depends on: 1081782
Depends on: 1081783
Depends on: 1081785
Depends on: 1081786
Depends on: 1081787
No longer depends on: 952875
No longer blocks: 1010718
Whiteboard: [grooming]
blocking-b2g: backlog → ---
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.