Closed Bug 935525 Opened 8 years ago Closed 7 years ago

[NFC] Marionette test cases for NFC

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S1 (14feb)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(7 files, 14 obsolete files)

14.68 KB, text/plain
Details
57.81 KB, text/plain
Details
35.57 KB, text/plain
Details
1.67 KB, text/plain
Details
2.28 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review
3.36 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.99 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
This depends on the emulator patches. Some basic tests should be OK for now.
Depends on: b2g-nfc
Attached file error.log (obsolete) —
Failed to build Gecko with the patches.

  The error occurred while processing the following file:
    /home/vicamo/WorkSpace/mozilla/b2g/qemu/jb/arm/master/2/gecko/dom/nfc/tests/moz.build
  The error was triggered on line 7 of this file:
    MODULE = 'test_dom_nfc'
Oopsi, bitrotted because of bug 93904. Should work now.
Attachment #831538 - Attachment is obsolete: true
Comment on attachment 8341683 [details] [diff] [review]
[01] Bug 935525: Added NFC test infrastructure to build scripts (v2)

Review of attachment 8341683 [details] [diff] [review]:
-----------------------------------------------------------------

have to add to testing/marionette/client/marionette/tests/unit-tests.ini as well
Comment on attachment 831540 [details] [diff] [review]
[02] Bug 935525: Added marionette test for turning NFC on and off

Review of attachment 831540 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/manifest.ini
@@ +2,5 @@
>  b2g=true
>  browser=false
>  qemu=true
> +
> +[test_nfc_enable.js]

test_nfc_enable"d".js
Comment on attachment 831542 [details] [diff] [review]
[03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered

Review of attachment 831542 [details] [diff] [review]:
-----------------------------------------------------------------

This part crashes emulator.  See https://github.com/mozilla-b2g/platform_external_qemu/pull/52#issuecomment-32143430
Depends on: 960790
Attachment #831540 - Attachment is obsolete: true
Attachment #8362899 - Flags: review?(allstars.chh)
Attachment #831542 - Attachment is obsolete: true
Attachment #8362900 - Flags: review?(allstars.chh)
Attachment #8341683 - Flags: review?(allstars.chh)
I saw Vicamo has already given some comments before, Vicamo, would you like to review this?
(I got some 1.3+ bugs and may not review these soon).
Attachment #8341683 - Flags: review?(allstars.chh) → review?(vyang)
Attachment #8362899 - Flags: review?(allstars.chh) → review?(vyang)
Attachment #8362900 - Flags: review?(allstars.chh) → review?(vyang)
Comment on attachment 8341683 [details] [diff] [review]
[01] Bug 935525: Added NFC test infrastructure to build scripts (v2)

Review of attachment 8341683 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 7.  Or these cases won't be activated by default.
Attachment #8341683 - Flags: review?(vyang)
Still get crashed after sending 'nfc ntf rf_intf_activated 0' either by test script or manually.
I found:

  I/USERIAL_LINUX(   47): USERIAL_Open(): enter
  I/USERIAL_LINUX(   47): USERIAL_Open() device: /dev/bcm2079x port=5, uart_port=0 WAKE_DELAY(0) WRITE_DELAY(0) POWER_ON_DELAY(300) PRE_POWER_OFF_DELAY(0) POST_POWER_OFF_DELAY(0)
  I/USERIAL_LINUX(   47): USERIAL_Open unable to open /dev/bcm2079x
  I/USERIAL_LINUX(   47): USERIAL_Open(): exit

Does that matter?
Attachment #8341482 - Attachment is obsolete: true
Attachment #8364182 - Attachment description: crash.adb-logcat.txt → crash.marionette.adb-logcat.txt
Attachment #8364181 - Attachment description: crash.emulator-console.txt → crash.manual.emulator-console.txt
Same assertion triggered.
Comment on attachment 8362899 [details] [diff] [review]
[02] Bug 935525: Added Marionette test for turning NFC on and off (v2)

Review of attachment 8362899 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/test_nfc_enabled.js
@@ +3,5 @@
> +
> +MARIONETTE_TIMEOUT = 30000;
> +
> +SpecialPowers.addPermission('nfc', true, document);
> +SpecialPowers.addPermission('settings', true, document);

We'd better not using `{add,remove}Permission` in newly committed code.  Please use `pushPermissions` instead.
Attachment #8362899 - Flags: review?(vyang)
Comment on attachment 8362900 [details] [diff] [review]
[03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v2)

Review of attachment 8362900 [details] [diff] [review]:
-----------------------------------------------------------------

This part still leads to emulator crash but probably should not be the one to blame.

::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js
@@ +3,5 @@
> +
> +MARIONETTE_TIMEOUT = 30000;
> +
> +SpecialPowers.addPermission('nfc-manager', true, document);
> +SpecialPowers.addPermission('settings', true, document);

ditto.
Attachment #8362900 - Flags: review?(vyang)
Comment on attachment 8362900 [details] [diff] [review]
[03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v2)

Review of attachment 8362900 [details] [diff] [review]:
-----------------------------------------------------------------

With fix in bug 916863, these test cases finally turn green!
Attachment #8362900 - Flags: feedback+
Added fixes from previous review.
Attachment #8341683 - Attachment is obsolete: true
Attachment #8366537 - Flags: review?(vyang)
Added fixes from previous review.
Attachment #8362899 - Attachment is obsolete: true
Attachment #8366541 - Flags: review?(vyang)
Added fixes from previous review.
Attachment #8362900 - Attachment is obsolete: true
Attachment #8366542 - Flags: review?(vyang)
Comment on attachment 8366537 [details] [diff] [review]
[01] Bug 935525: Added NFC test infrastructure to build scripts (v3)

Review of attachment 8366537 [details] [diff] [review]:
-----------------------------------------------------------------

Ask :jgriffin's review as well.
Attachment #8366537 - Flags: review?(vyang)
Attachment #8366537 - Flags: review?(jgriffin)
Attachment #8366537 - Flags: review+
Comment on attachment 8366541 [details] [diff] [review]
[02] Bug 935525: Added Marionette test for turning NFC on and off (v3)

Review of attachment 8366541 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/test_nfc_enabled.js
@@ +63,5 @@
> +  finish();
> +}
> +
> +SpecialPowers.pushPermissions(
> +  [{'type':'settings', 'allow':true, 'context':document}], runNextTest());

s/runNextTest()/runNextTest/
Attachment #8366541 - Flags: review?(vyang) → review-
Comment on attachment 8366542 [details] [diff] [review]
[03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v3)

Review of attachment 8366542 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js
@@ +73,5 @@
> +}
> +
> +SpecialPowers.pushPermissions(
> +  [{'type':'nfc-manager', 'allow':true, context:document},
> +   {'type':'settings', 'allow':true, context:document}], runNextTest());

ditto
Attachment #8366542 - Flags: review?(vyang) → review-
Attachment #8366537 - Flags: review?(jgriffin) → review+
Depends on: 968063
Attachment #8370822 - Flags: review?(vyang) → review+
Comment on attachment 8370823 [details] [diff] [review]
[03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v4)

Review of attachment 8370823 [details] [diff] [review]:
-----------------------------------------------------------------

All verified with current emulator-jb build.  Thank you!
Attachment #8370823 - Flags: review?(vyang) → review+
Comment on attachment 8366537 [details] [diff] [review]
[01] Bug 935525: Added NFC test infrastructure to build scripts (v3)

Review of attachment 8366537 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/manifest.ini
@@ +1,4 @@
> +[DEFAULT]
> +b2g=true
> +browser=false
> +qemu=true

We can only run these tests on emulator-jb or above, so there are still something to do here. :(
Attachment #8366537 - Flags: review+
backedout from inbound on request from tdz in 855c5a0c6828
(In reply to Carsten Book [:Tomcat] from comment #31)
> backedout from inbound on request from tdz in 855c5a0c6828

Oops, I don't know they're already in inbound :X
v5 of the Marionette tests immediately succeed on systems without NFC. This makes them work on both, emulator and emulator-js.
s/emulator-js/emulator-jb
Comment on attachment 8372285 [details] [diff] [review]
[02] Bug 935525: Added Marionette test for turning NFC on and off (v5)

Review of attachment 8372285 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/test_nfc_enabled.js
@@ +64,5 @@
> +  if ('mozNfc' in window.navigator) {
> +    runNextTest();
> +  } else {
> +    // succeed immediately on systems without NFC
> +    ok(true, 'Failed on system without NFC');

s/Failed/Skipped/.  Please also have a |log(...)| line to print on console directly.  |ok(...)| is only printed in output of `adb logcat`
Attachment #8372285 - Flags: review?(vyang) → review+
Comment on attachment 8372287 [details] [diff] [review]
[03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v5)

Review of attachment 8372287 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js
@@ +73,5 @@
> +  if ('mozNfc' in window.navigator) {
> +    runNextTest();
> +  } else {
> +    // succeed immediately on systems without NFC
> +    ok(true, 'Failed on system without NFC');

ditto.
Attachment #8372287 - Flags: review?(vyang) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #37)
> Comment on attachment 8372285 [details] [diff] [review]
> [02] Bug 935525: Added Marionette test for turning NFC on and off (v5)
> 
> Review of attachment 8372285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/nfc/tests/marionette/test_nfc_enabled.js
> @@ +64,5 @@
> > +  if ('mozNfc' in window.navigator) {
> > +    runNextTest();
> > +  } else {
> > +    // succeed immediately on systems without NFC
> > +    ok(true, 'Failed on system without NFC');
> 
> s/Failed/Skipped/.  Please also have a |log(...)| line to print on console
> directly.  |ok(...)| is only printed in output of `adb logcat`

Correct me if I'm wrong, but it seems that this 'bogus' test is necessary. When I called finish() directly, Marionette complained that no tests have been executed. So I added the ok(), and 'Failed...' is just the error message.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #39)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #37)
> > s/Failed/Skipped/.  Please also have a |log(...)| line to print on console
> > directly.  |ok(...)| is only printed in output of `adb logcat`
> 
> Correct me if I'm wrong, but it seems that this 'bogus' test is necessary.
> When I called finish() directly, Marionette complained that no tests have
> been executed. So I added the ok(), and 'Failed...' is just the error
> message.

Yes, that's necessary, and that's why I used "also".  :)
OK, next round. I added some logging to the tests. I also moved the generic code into a separate file.
Attachment #8372285 - Attachment is obsolete: true
Attachment #8373909 - Flags: review?(vyang)
Attachment #8373909 - Flags: review?(vyang) → review+
Comment on attachment 8373910 [details] [diff] [review]
[03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v6)

Review of attachment 8373910 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js
@@ +13,5 @@
> +
> +function activateRE0() {
> +  var cmd = 'nfc ntf rf_intf_activated 0';
> +  log('Executing \'' + cmd + '\'');
> +  runEmulatorCmd(cmd, function(result) {

Should have noticed.  Whenever we're calling built-in `runEmulatorCmd()`, we have to manually keep track of its execution count and make sure all of the calls are already finished before calling `finish()` at the end of the test script.  Please see https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/head.js#270 for example.
Attachment #8373910 - Flags: review?(vyang) → review+
Ok, great. I'll update and land the patches then. Thanks a lot!
https://hg.mozilla.org/mozilla-central/rev/6e1e721b044a
https://hg.mozilla.org/mozilla-central/rev/43b33c890e57
https://hg.mozilla.org/mozilla-central/rev/5dc42ca15601
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S1 (14feb)
You need to log in before you can comment on or make changes to this bug.