935525 - [NFC] Marionette test cases for NFC

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This depends on the emulator patches. Some basic tests should be OK for now.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 935525: Added NFC test infrastructure to build scripts

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 935525: Added marionette test for turning NFC on and off

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: error.log

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'

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 935525: Added NFC test infrastructure to build scripts (v2)

Oopsi, bitrotted because of bug 93904. Should work now.

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

bug 939044

Comment 7

[Vicamo Yang [:vicamo][: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]:
-----------------------------------------------------------------

have to add to testing/marionette/client/marionette/tests/unit-tests.ini as well

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

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 9

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 935525: Added Marionette test for turning NFC on and off (v2)

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v2)

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 13

[Vicamo Yang [:vicamo][: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.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: crash.manual.emulator-console.txt

Still get crashed after sending 'nfc ntf rf_intf_activated 0' either by test script or manually.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: crash.marionette.adb-logcat.txt

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?

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: crash.manual.adb-logcat.txt

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: crash.manual.gdb-backtrace.txt

Same assertion triggered.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 19

[Vicamo Yang [:vicamo][: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.

Comment 20

[Vicamo Yang [:vicamo][: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!

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 935525: Added NFC test infrastructure to build scripts (v3)

Added fixes from previous review.

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 935525: Added Marionette test for turning NFC on and off (v3)

Added fixes from previous review.

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v3)

Added fixes from previous review.

Comment 24

[Vicamo Yang [:vicamo][: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.

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

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/

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 935525: Added Marionette test for turning NFC on and off (v4)

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v4)

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

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!

Comment 30

[Vicamo Yang [:vicamo][: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]:
-----------------------------------------------------------------

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

Comment 31

[Carsten Book [:Tomcat]]

backedout from inbound on request from tdz in 855c5a0c6828

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

(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

Comment 33

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 935525: Added Marionette test for turning NFC on and off (v5)

Comment 34

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v5)

Comment 35

[Thomas Zimmermann [:tzimmermann] [:tdz]]

v5 of the Marionette tests immediately succeed on systems without NFC. This makes them work on both, emulator and emulator-js.

Comment 36

[Thomas Zimmermann [:tzimmermann] [:tdz]]

s/emulator-js/emulator-jb

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

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`

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 39

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 41

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 935525: Added Marionette test for turning NFC on and off (v6)

OK, next round. I added some logging to the tests. I also moved the generic code into a separate file.

Comment 42

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v6)

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 44

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ok, great. I'll update and land the patches then. Thanks a lot!

Comment 45

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 935525: Added Marionette test for nfc-manager-tech-discovered (v7)

Comment 46

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6e1e721b044a
https://hg.mozilla.org/integration/b2g-inbound/rev/43b33c890e57
https://hg.mozilla.org/integration/b2g-inbound/rev/5dc42ca15601

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=5dc42ca15601

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e1e721b044a
https://hg.mozilla.org/mozilla-central/rev/43b33c890e57
https://hg.mozilla.org/mozilla-central/rev/5dc42ca15601