Closed Bug 907252 Opened 6 years ago Closed 4 years ago

B2G NFC: write xpcshell-test for NFC

Categories

(Firefox OS Graveyard :: NFC, defect)

x86_64
Linux
defect
Not set

Tracking

(tracking-b2g:backlog, firefox48 fixed)

RESOLVED FIXED
tracking-b2g backlog
Tracking Status
firefox48 --- fixed

People

(Reporter: allstars.chh, Assigned: tnguyen)

References

Details

Attachments

(1 file, 7 obsolete files)

We need some tests for testing nfc_worker,
and we could start from xpcshell tests.
Blocks: 902051
No longer blocks: b2g-nfc
Blocks: 897312
Component: DOM: Device Interfaces → NFC
Product: Core → Boot2Gecko
Assignee: nobody → dlee
Blocks: b2g-NFC-2.0
Blocks: 986527
No longer blocks: b2g-NFC-2.0
blocking-b2g: --- → backlog
No longer blocks: 897312
Even we don't have nfc_worker for now.
I'd still try if we can have some tests for Nfc.js, or perhaps NfcContentHelper.js.
Assignee: dlee → allstars.chh
No longer blocks: b2g-nfc
blocking-b2g: backlog → ---
Assignee: allstars.chh → tnguyen
Attached patch xpcshell test Nfc.js v1 (obsolete) — Splinter Review
Add some xpcshell tests for Nfc.js
Attachment #8694099 - Flags: review?(allstars.chh)
Status: NEW → ASSIGNED
Comment on attachment 8694099 [details] [diff] [review]
xpcshell test Nfc.js v1

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

There should be other tests for Tag/Peer, with a mock NfcService,
if you don't plan to do it in this bug, file another bug.

::: dom/nfc/tests/unit/header_helpers.js
@@ +25,5 @@
> +
> +function waitAsyncMessage(msg, callback) {
> +  let handler = {
> +    receiveMessage: function(message) {
> +      if (message.name === msg) {

bail out early

@@ +35,5 @@
> +
> +  cpmm.addMessageListener(msg, handler);
> +}
> +
> +function sendAsyncMessage(msg, rsp, callback, payload) {

callback should be the last arg

@@ +36,5 @@
> +  cpmm.addMessageListener(msg, handler);
> +}
> +
> +function sendAsyncMessage(msg, rsp, callback, payload) {
> +  if (rsp) {

Will rsp be null?
If not why this check?

::: dom/nfc/tests/unit/test_Nfc.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +var NFC = {};

let

@@ +9,5 @@
> +
> +var messageManager = NFC.gMessageManager;
> +var sessionHelper = NFC.SessionHelper;
> +
> +sessionHelper.tokenMap = {};

is this line neccesary?

@@ +10,5 @@
> +var messageManager = NFC.gMessageManager;
> +var sessionHelper = NFC.SessionHelper;
> +
> +sessionHelper.tokenMap = {};
> +messageManager._registerMessageListeners();

Is _unregisterMessageListeners missing or it could be skipped?

@@ +23,5 @@
> +  let rsp = "NFC:DOMEvent";
> +  let callback = function(message) {
> +    let result = message.data;
> +
> +    equal(NFC_CONSTS.FOCUS_CHANGED, result.event);

equal(value, expectedValue);

so it should be 
equal(result.event, NFC_CONSTS.FOCUS_CHANGED);

However it would be simpler if you use a JS object and use deepEqual to compare it.

@@ +26,5 @@
> +
> +    equal(NFC_CONSTS.FOCUS_CHANGED, result.event);
> +    equal(isFocus, result.focus);
> +    equal(tabId, result.tabId);
> +    equal(tabId, messageManager.focusId);

this check is trival.

@@ +39,5 @@
> +                   { tabId: tabId,
> +                     isFocus: isFocus });
> +});
> +
> +add_test(function test_setFocusTabOff() {

could it be merged with test_setFocusTabOn?

@@ +71,5 @@
> +  let isP2P = true;
> +  let callback = function(message) {
> +    let result = message.data;
> +
> +    equal(requestId, result.requestId);

should check errorMsg as well.

@@ +84,5 @@
> +  sendSyncMessage("NFC:RegisterPeerReadyTarget", { appId: appId });
> +  sendAsyncMessage("NFC:CheckP2PRegistration", rsp, callback,
> +                   { appId: appId,
> +                     requestId: requestId });
> +});

There should be a case for checkP2PRegistration returns false.

@@ +97,5 @@
> +    let result = message.data;
> +
> +    equal(NFC_CONSTS.PEER_EVENT_READY, result.event);
> +    equal(sessionHelper.getCurrentP2PToken(), result.sessionToken);
> +    equal(messageManager.getFocusTabId(), result.tabId);

these checks are trival.

@@ +129,5 @@
> +  };
> +
> +  sendSyncMessage("NFC:AddEventListener", { tabId: tabId });
> +  waitAsyncMessage(msg, callback);
> +  messageManager.onRFStateChanged(rfState);

should use NFC::ChangeRFState
Attachment #8694099 - Flags: review?(allstars.chh)
Comment on attachment 8694099 [details] [diff] [review]
xpcshell test Nfc.js v1

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

::: dom/nfc/tests/unit/header_helpers.js
@@ +24,5 @@
> +                                   "nsIUUIDGenerator");
> +
> +function waitAsyncMessage(msg, callback) {
> +  let handler = {
> +    receiveMessage: function(message) {

nit, function (message)

::: dom/nfc/tests/unit/test_Nfc.js
@@ +20,5 @@
> +add_test(function test_setFocusTabOn() {
> +  let tabId = 1;
> +  let isFocus = true;
> +  let rsp = "NFC:DOMEvent";
> +  let callback = function(message) {

nit, function (message)
There should be a space after function,
same for below.
> > +var messageManager = NFC.gMessageManager;
> > +var sessionHelper = NFC.SessionHelper;
> > +
> > +sessionHelper.tokenMap = {};
> 
> is this line neccesary?

Yep, otherwise, tokenMap would be "undefined" later

> 
> @@ +84,5 @@
> > +  sendSyncMessage("NFC:RegisterPeerReadyTarget", { appId: appId });
> > +  sendAsyncMessage("NFC:CheckP2PRegistration", rsp, callback,
> > +                   { appId: appId,
> > +                     requestId: requestId });
> > +});
> 
> There should be a case for checkP2PRegistration returns false.

I skipped this case due to the following line
      if (!isValid) {
        respMsg.errorMsg = this.nfc.getErrorMessage(NFC.NFC_GECKO_ERROR_P2P_REG_INVALID);
      }
nfc variable is undefined, I still don't know how to construct it (the construct method has private access)



> > +  };
> > +
> > +  sendSyncMessage("NFC:AddEventListener", { tabId: tabId });
> > +  waitAsyncMessage(msg, callback);
> > +  messageManager.onRFStateChanged(rfState);
> 
> should use NFC::ChangeRFState

Similar as above, changeRFState would call this.nfc.receiveMessage(message) and nfc is undefined
> Comment on attachment 8694099 [details] [diff] [review]
> xpcshell test Nfc.js v1
> 
> Review of attachment 8694099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There should be other tests for Tag/Peer, with a mock NfcService,
> if you don't plan to do it in this bug, file another bug.
> 
Could you tell more details?
Flags: needinfo?(allstars.chh)
Attached patch xpcshell test Nfc.js v2 (obsolete) — Splinter Review
Attachment #8694099 - Attachment is obsolete: true
Attached patch 8702238: xpcshell test Nfc.js v2 (obsolete) — Splinter Review
Add missing file to the patch :)
Attachment #8702238 - Attachment is obsolete: true
Attachment #8702239 - Flags: review?(allstars.chh)
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #6)
> > Comment on attachment 8694099 [details] [diff] [review]
> > xpcshell test Nfc.js v1
> > 
> > Review of attachment 8694099 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > There should be other tests for Tag/Peer, with a mock NfcService,
> > if you don't plan to do it in this bug, file another bug.
> > 
> Could you tell more details?

Like what you did for messageManager and sessionHelper in your tests.
Flags: needinfo?(allstars.chh)
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #5)
> > 
> > @@ +84,5 @@
> > > +  sendSyncMessage("NFC:RegisterPeerReadyTarget", { appId: appId });
> > > +  sendAsyncMessage("NFC:CheckP2PRegistration", rsp, callback,
> > > +                   { appId: appId,
> > > +                     requestId: requestId });
> > > +});
> > 
> > There should be a case for checkP2PRegistration returns false.
> 
> I skipped this case due to the following line
>       if (!isValid) {
>         respMsg.errorMsg =
> this.nfc.getErrorMessage(NFC.NFC_GECKO_ERROR_P2P_REG_INVALID);
>       }
> nfc variable is undefined, I still don't know how to construct it (the
> construct method has private access)
> 
> 
> 
> > > +  };
> > > +
> > > +  sendSyncMessage("NFC:AddEventListener", { tabId: tabId });
> > > +  waitAsyncMessage(msg, callback);
> > > +  messageManager.onRFStateChanged(rfState);
> > 
> > should use NFC::ChangeRFState
> 
> Similar as above, changeRFState would call this.nfc.receiveMessage(message)
> and nfc is undefined


Can't this be done like what you did for 'sessionHelper.tokenMap = {};'?
To set nfc in gMessageManager.
Comment on attachment 8702239 [details] [diff] [review]
8702238: xpcshell test Nfc.js v2

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

::: dom/nfc/tests/unit/header_helpers.js
@@ +7,5 @@
> +/* exported run_test */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +var NFC_CONSTS = {};

let, and everywhere else

@@ +26,5 @@
> +function waitAsyncMessage(msg, callback) {
> +  let handler = {
> +    receiveMessage: function (message) {
> +      if (message.name !== msg) {
> +          return;

indent

::: dom/nfc/tests/unit/test_Nfc.js
@@ +24,5 @@
> +    let result = message.data;
> +
> +    equal(result.event, NFC_CONSTS.FOCUS_CHANGED);
> +    equal(result.focus, true);
> +    equal(result.tabId, tabId);

Can't this use deepEqual?
deepEqual(result, {...});

@@ +70,5 @@
> +  sendSyncMessage("NFC:RegisterPeerReadyTarget", { appId: appId });
> +  sendAsyncMessage("NFC:CheckP2PRegistration", rsp, {
> +                     appId: appId,
> +                     requestId: requestId
> +                   }, callback);

sendAsyncMessage("NFC:CheckP2PRegistration",
                 rsp,
                 {...,
                  ....},
                 callback);

@@ +143,5 @@
> +  sendSyncMessage("NFC:AddEventListener", { tabId: tabId });
> +  sendAsyncMessage("NFC:SetFocusTab", rsp, {
> +                     tabId: tabId,
> +                     isFocus: isFocus
> +                   }, setFocusCb);

ditto,
Attachment #8702239 - Flags: review?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh] from comment #10)
> > Similar as above, changeRFState would call this.nfc.receiveMessage(message)
> > and nfc is undefined
> 
> 
> Can't this be done like what you did for 'sessionHelper.tokenMap = {};'?
> To set nfc in gMessageManager.
I use 

subscriptLoader.loadSubScript("resource://gre/components/Nfc.js", NFC);


AFAIK, The mozIJSSubScriptLoader only allows global variable and function created by loading script to be defined as properties of target object (in other words, only global variable and function in Nfc.js could be used). 
Nfc constructor and prototype have private access (and mozIJSSubScriptLoader could not load them) so nfc in gMessageManager would not be able to be initialized  using constructor in Nfc.js (unless we copy this block of code to test script, I don't think it's a good idea for an unit test). 

(In reply to Yoshi Huang[:allstars.chh] from comment #9)
> (In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #6)
> > > 
> > Could you tell more details?
> 
> Like what you did for messageManager and sessionHelper in your tests.

I think that has the same reason as above. 

Correct me if I am wrong and let me know if you have any suggestion.
Attached patch xpcshell test Nfc.js v23 (obsolete) — Splinter Review
Attachment #8702239 - Attachment is obsolete: true
Attachment #8702831 - Flags: review?(allstars.chh)
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #5)
> > 
> > @@ +84,5 @@
> > > +  sendSyncMessage("NFC:RegisterPeerReadyTarget", { appId: appId });
> > > +  sendAsyncMessage("NFC:CheckP2PRegistration", rsp, callback,
> > > +                   { appId: appId,
> > > +                     requestId: requestId });
> > > +});
> > 
> > There should be a case for checkP2PRegistration returns false.
> 
> I skipped this case due to the following line
>       if (!isValid) {
>         respMsg.errorMsg =
> this.nfc.getErrorMessage(NFC.NFC_GECKO_ERROR_P2P_REG_INVALID);
>       }
> nfc variable is undefined, I still don't know how to construct it (the
> construct method has private access)
> 
We didn't use closure so there shouldn't be any private member.
Try to construct this.nfc.
Comment on attachment 8702831 [details] [diff] [review]
xpcshell test Nfc.js v23

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

comment 14
Attachment #8702831 - Flags: review?(allstars.chh)
> We didn't use closure so there shouldn't be any private member.
> Try to construct this.nfc.
You're right, thanks for your help on this bug.

I got nfc initialized but somehow the test always failed.
After working a bit and found that the line 157
      if (!NFC.DEBUG_NFC) {
        let lock = gSettingsService.createLock();
        lock.get(NFC.SETTING_NFC_DEBUG, this.nfc);
        Services.obs.addObserver(this, NFC.TOPIC_MOZSETTINGS_CHANGED, false);
      }
causes xpcshell test could not exit due to the lock (even it ran all the tests PASSED), then time-out
I'm not very sure about sevicesetting lock mechanism then, I have to set 
NFC.NFC.DEBUG_NFC = true to pass these lines to create more test cases,
Firstly set a ni to you to ask your idea for that.
Thanks
Flags: needinfo?(allstars.chh)
check why SettingsService causes timeout in xpcshell-test.
Flags: needinfo?(allstars.chh)
Attached patch xpcshell test v3 (obsolete) — Splinter Review
Attachment #8713448 - Flags: review?(allstars.chh)
Attachment #8702831 - Attachment is obsolete: true
Attachment #8713448 - Attachment description: 0001-Bug-907252-B2G-NFC-write-xpcshell-test-for-NFC.-r-yo.patch → xpcshell test v3
Comment on attachment 8713448 [details] [diff] [review]
xpcshell test v3

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

::: dom/nfc/gonk/Nfc.js
@@ +149,5 @@
>      eventListeners: {},
>  
>      focusId: NFC.SYSTEM_APP_ID,
>  
> +    init: function init(nfc, debug) {

So you add another argument 'debug' here,
however it makes some confusion there as it mixes some *unrelated* information together, 
1. calling from test_Nfc.js
2. when the flag DEBUG_NFC is false.

That makes code hard to read and hard to understand what you're trying to fix here,
I suggest following:

@@ +160,5 @@
>        }
>  
>        Services.obs.addObserver(this, NFC.TOPIC_XPCOM_SHUTDOWN, false);
>        this._registerMessageListeners();
>      },

init: function init(nfc) {
  this.nfc = nfc;
  Services.obs.addObserver(this, NFC.TOPIC_XPCOM_SHUTDOWN, false);
  this._registerMessageListeners();
},

listenDebugEvent: function listenDebugEvent() {
  let lock = gSettingsService.createLock();
  lock.get(NFC.SETTING_NFC_DEBUG, this.nfc);
  Services.obs.addObserver(this, NFC.TOPIC_MOZSETTINGS_CHANGED, false);
}

@@ +515,3 @@
>  
>    this.targetsByRequestId = {};
>  }

function Nfc(isXPCShell) {
  // TODO: Bug 1239954: .....
  // gSettingsService.createLock will cause timeout while running xpshell-test, so we try to prevent to run gSettingsService under xpcshell-test here.
  gMessageManager.init(this);
  if (!isXPCShell && !NFC.DEBUG_NFC) {
    gMessageManager.listenDebugEvent();
  }

  this.targetsByRequestId = {};
}

::: dom/nfc/tests/unit/test_Nfc.js
@@ +3,5 @@
> +
> +"use strict";
> +
> +let NFC = {};
> +

extra line

@@ +8,5 @@
> +subscriptLoader.loadSubScript("resource://gre/components/Nfc.js", NFC);
> +
> +// Mock nfc service
> +// Type define should be consistent with type define in Nfc.js
> +const MockReqType = {

Couldn't we use NFC.NfcRequestType?
Same for response and notification below.

@@ +37,5 @@
> +                                 "6":111,"7":122,"8":105},
> +                      "tnf":"well-known",
> +                      "type":{"0":85}};
> +
> +var MockNfcService = {

let

@@ +44,5 @@
> +  setListener: function(listener) {
> +    this.listener = listener;
> +  },
> +
> +  notifyEvent: function (event) {

I notice that you use two styles in between.

function(..)
and
function (...)

one with an extra space after function, and the other is not.

Choose one and use it consistenly.

@@ +92,5 @@
> +
> +let messageManager = NFC.gMessageManager;
> +let sessionHelper = NFC.SessionHelper;
> +
> +messageManager.nfc = new NFC.Nfc(true);

should be just
new NFC.nfc(/* isXPCShell */ true);

assigning messageManager.nfc is not neccesary.

@@ +173,5 @@
> +add_test(function test_checkP2PRegistrationFailed() {
> +  let appId = 1;
> +  let requestId = 10;
> +  let rsp = "NFC:CheckP2PRegistrationResponse";
> +  let errorMsg = 

trailing ws
Attachment #8713448 - Flags: review?(allstars.chh) → review+
Attached patch xpcshell patch v4 (obsolete) — Splinter Review
Thank a lot for looking into this.
I updated as your suggestion except
> > +
> > +// Mock nfc service
> > +// Type define should be consistent with type define in Nfc.js
> > +const MockReqType = {
> 
> Couldn't we use NFC.NfcRequestType?
> Same for response and notification below.

I tried and it failed. Things declared via let and const could not be used by loadSubscript ( as said in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIJSSubScriptLoader). 

Thanks again.
Attachment #8713448 - Attachment is obsolete: true
Attachment #8713514 - Flags: review+
Comment on attachment 8713514 [details] [diff] [review]
xpcshell patch v4

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

::: dom/nfc/gonk/Nfc.js
@@ +508,5 @@
>    }
>  };
>  
> +function Nfc(isXPCShell) {
> +  // TODO: Bug 1239954: .....

You should fill out the bug title here.
Attached patch XpcShell test case (obsolete) — Splinter Review
Attachment #8713514 - Attachment is obsolete: true
Attachment #8714162 - Flags: review+
Assignee: tnguyen → nobody
Status: ASSIGNED → NEW
Keywords: checkin-needed
Assignee: nobody → tnguyen
abort: bad hunk #2 @@ -504,18 +503,24 @@ var SessionHelper = {
 (18 18 25 24)
Keywords: checkin-needed
Attached patch Test caseSplinter Review
Rebased and re-attached
Attachment #8714162 - Attachment is obsolete: true
Attachment #8730070 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ce7b7e13c3f1
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.