Closed
Bug 1035606
Opened 8 years ago
Closed 8 years ago
[NFC] Testcase cleanup
Categories
(Firefox OS Graveyard :: NFC, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S6 (18july)
People
(Reporter: dimi, Assigned: dimi)
References
Details
(Whiteboard: [p=2])
Attachments
(1 file, 2 obsolete files)
20.65 KB,
patch
|
dimi
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8452198 -
Flags: review?(tzimmermann)
Comment 2•8 years ago
|
||
Comment on attachment 8452198 [details] [diff] [review] bug 1035606 patch v1 Review of attachment 8452198 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup! :) I have a few points, you might consider cleaning up as well, but this looks good already. ::: dom/nfc/tests/marionette/head.js @@ +14,5 @@ > +const P2P_RE_INDEX_1 = 1; > +const T1T_RE_INDEX = 2; > +const T2T_RE_INDEX = 3; > +const T3T_RE_INDEX = 4; > +const T4T_RE_INDEX = 5; Would it make sense to move these constants into the emulator object? @@ +119,5 @@ > + }; > +}()); > + > +let SNEP = (function() { > + function putNdef(dsap, ssap, flags, tnf, type, payload, id) { Maybe just |put|, like the protocol operation? @@ +173,5 @@ > window.navigator.mozSetMessageHandler(type, null); > }); > } > > function enableRE0() { This function should be removed and users should be converted to |NCI.activateRE|. ::: dom/nfc/tests/marionette/test_nfc_read_tag.js @@ +94,5 @@ > testUrlT4TDiscover, > testEmptyT1TDiscover, > testEmptyT2TDiscover, > testEmptyT3TDiscover, > + testEmptyT4TDiscover, Just out of curiosity, is there a difference between having the comma and not having it?
Attachment #8452198 -
Flags: review?(tzimmermann) → review+
Assignee | ||
Comment 3•8 years ago
|
||
> ::: dom/nfc/tests/marionette/test_nfc_read_tag.js
> @@ +94,5 @@
> > testUrlT4TDiscover,
> > testEmptyT1TDiscover,
> > testEmptyT2TDiscover,
> > testEmptyT3TDiscover,
> > + testEmptyT4TDiscover,
>
> Just out of curiosity, is there a difference between having the comma and
> not having it?
No, it works both way. Just forget to remove it after removing some debug code.
I'll remove it in next patch.
Assignee | ||
Comment 4•8 years ago
|
||
Hi Thomas, I did other clean up according to your suggestion. could you help review again? thanks!
Attachment #8452198 -
Attachment is obsolete: true
Attachment #8452846 -
Flags: review?(tzimmermann)
Comment 6•8 years ago
|
||
Dimi you will also need to rebase and fix test_nfc_error_messages.js once Bug 1033218 will land. I think it will be conflicted with the current patch because it uses emulator.activateRE and adds emulator.deactivate usage instead of toggleNFC(false).
Depends on: 1033218
Comment 7•8 years ago
|
||
Comment on attachment 8452846 [details] [diff] [review] bug 1035606 v2 patch Review of attachment 8452846 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: dom/nfc/tests/marionette/test_nfc_manager_tech_discovered.js @@ +31,5 @@ > window.navigator.mozSetMessageHandler( > 'nfc-manager-tech-discovered', handleTechnologyDiscoveredRE0); > > toggleNFC(true) > + .then(() => NCI.notifyDiscoverRE(emulator.P2P_RE_INDEX_0, NCI_MORE_NOTIFICATIONS)) I just noticed that these constants could be moved into NCI without the prefix. Are there any other constants anywhere?
Attachment #8452846 -
Flags: review?(tzimmermann) → review+
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #6) > Dimi you will also need to rebase and fix test_nfc_error_messages.js once > Bug 1033218 will land. I think it will be conflicted with the current patch > because it uses emulator.activateRE and adds emulator.deactivate usage > instead of toggleNFC(false). Thanks for the info, I will rebase it once bug 1033218 merges to the central
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8452846 -
Attachment is obsolete: true
Attachment #8453520 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Whiteboard: [p=2]
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•8 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4bb02b0257b5
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
Slight overkill there, but thanks ;) https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices https://hg.mozilla.org/integration/b2g-inbound/rev/97ed5d7ce52f
Flags: in-testsuite+
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/97ed5d7ce52f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•