Closed
Bug 907252
Opened 11 years ago
Closed 8 years ago
B2G NFC: write xpcshell-test for NFC
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(tracking-b2g:backlog, firefox48 fixed)
People
(Reporter: allstars.chh, Assigned: tnguyen)
References
Details
Attachments
(1 file, 7 obsolete files)
19.18 KB,
patch
|
tnguyen
:
review+
|
Details | Diff | Splinter Review |
We need some tests for testing nfc_worker, and we could start from xpcshell tests.
Updated•11 years ago
|
Updated•11 years ago
|
Component: DOM: Device Interfaces → NFC
Product: Core → Boot2Gecko
Updated•11 years ago
|
Assignee: nobody → dlee
Updated•10 years ago
|
Blocks: b2g-NFC-2.0
Updated•10 years ago
|
Updated•10 years ago
|
blocking-b2g: --- → backlog
Reporter | ||
Comment 1•10 years ago
|
||
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
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Assignee | ||
Updated•8 years ago
|
Assignee: allstars.chh → tnguyen
Assignee | ||
Comment 2•8 years ago
|
||
Add some xpcshell tests for Nfc.js
Assignee | ||
Updated•8 years ago
|
Attachment #8694099 -
Flags: review?(allstars.chh)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
> > +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
Assignee | ||
Comment 6•8 years ago
|
||
> 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)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8694099 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Add missing file to the patch :)
Assignee | ||
Updated•8 years ago
|
Attachment #8702238 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8702239 -
Flags: review?(allstars.chh)
Reporter | ||
Comment 9•8 years ago
|
||
(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)
Reporter | ||
Comment 10•8 years ago
|
||
(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.
Reporter | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
(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.
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8702239 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8702831 -
Flags: review?(allstars.chh)
Reporter | ||
Comment 14•8 years ago
|
||
(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.
Reporter | ||
Comment 15•8 years ago
|
||
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)
Assignee | ||
Comment 16•8 years ago
|
||
> 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)
Reporter | ||
Comment 17•8 years ago
|
||
check why SettingsService causes timeout in xpcshell-test.
Flags: needinfo?(allstars.chh)
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8713448 -
Flags: review?(allstars.chh)
Assignee | ||
Updated•8 years ago
|
Attachment #8702831 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8713448 -
Attachment description: 0001-Bug-907252-B2G-NFC-write-xpcshell-test-for-NFC.-r-yo.patch → xpcshell test v3
Reporter | ||
Comment 19•8 years ago
|
||
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+
Assignee | ||
Comment 20•8 years ago
|
||
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+
Reporter | ||
Comment 21•8 years ago
|
||
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.
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8713514 -
Attachment is obsolete: true
Attachment #8714162 -
Flags: review+
Assignee | ||
Comment 23•8 years ago
|
||
try:https://treeherder.mozilla.org/#/jobs?repo=try&revision=962f23530028&exclusion_profile=false&selectedJob=16184921
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → tnguyen
Comment 24•8 years ago
|
||
abort: bad hunk #2 @@ -504,18 +503,24 @@ var SessionHelper = { (18 18 25 24)
Keywords: checkin-needed
Assignee | ||
Comment 25•8 years ago
|
||
Rebased and re-attached
Attachment #8714162 -
Attachment is obsolete: true
Attachment #8730070 -
Flags: review+
Assignee | ||
Comment 26•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33614e634fa1
Keywords: checkin-needed
Comment 27•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7b7e13c3f1e9b9fa07015bd98f4b793bde751f Bug 907252 - B2G NFC: write xpcshell-test for NFC. r=yoshi
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ce7b7e13c3f1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•