Closed Bug 1019668 Opened 11 years ago Closed 10 years ago

[NFC] Nfc.js - do not add errorMsg to techDiscovered and techLost, reenable marionette tests

Categories

(Firefox OS Graveyard :: NFC, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox31 wontfix, firefox32 wontfix, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: tauzen, Assigned: tauzen)

References

Details

Attachments

(2 files, 7 obsolete files)

In case of techDiscoverd and techLost message from nfc worker we do not have a status property. This causes adding the generic errorMsg to the message, before sending it to upper layers. This should be removed.
Dimi if it's ok with you, I will reenable the missing marionette tests case, as I will be needing them in this bug.
Assignee: nobody → kmioduszewski
Blocks: b2g-nfc
Flags: needinfo?(dlee)
No problem, sorry for removing it accidentally before :(
Flags: needinfo?(dlee)
Tested on gecko f297dd29f28bb736dfcfd374dfc62c02ed4f871c and works fine. Retest needed once latest emulator will be fixed. Should be applied after Bug 1019436 to have better performance.
Attachment #8435090 - Flags: review?(allstars.chh)
Depends on: 1019436
Summary: [NFC] Nfc.js - Do not try to map message.status to errorMsg if status is undefined → [NFC] Nfc.js - do not add errorMsg to techDiscovered and techLost, reenable marionette tests
Comment on attachment 8435090 [details] [diff] [review] 0001-Bug-1019668-NFC-Nfc.js-do-not-add-errorMsg-to-techDi.patch Review of attachment 8435090 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/Nfc.js @@ +499,2 @@ > message.errorMsg = this.getErrorMessage(message.status); > } Move this if clause to messages handlers with the 'status' property, or if (message.status) or if (message.status !== undefined && message.status !== NFC.NFC_SUCCESS) if you want to make it more clear.
Attachment #8435090 - Flags: review?(allstars.chh)
Comment on attachment 8435090 [details] [diff] [review] 0001-Bug-1019668-NFC-Nfc.js-do-not-add-errorMsg-to-techDi.patch Review of attachment 8435090 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/manifest.ini @@ +9,5 @@ > [test_nfc_peer.js] > [test_nfc_peer_sendndef.js] > [test_nfc_tag.js] > +[test_nfc_checkP2PRegistration.js] > +[test_nfc_error_messages.js] Dimi said he removed these by accident before, so I'd suggest move this to another patch or bug.
Attachment #8435090 - Attachment is obsolete: true
Attachment #8436884 - Flags: review?(allstars.chh)
Attachment #8436886 - Flags: review?(allstars.chh)
white spaces removed
Attachment #8436884 - Attachment is obsolete: true
Attachment #8436884 - Flags: review?(allstars.chh)
Attachment #8436898 - Flags: review?(allstars.chh)
Same here, white spaces removed.
Attachment #8436886 - Attachment is obsolete: true
Attachment #8436886 - Flags: review?(allstars.chh)
Attachment #8436899 - Flags: review?(allstars.chh)
Comment on attachment 8436898 [details] [diff] [review] Part:1 Not adding errorMsg if status undefined Review of attachment 8436898 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/nfc/tests/marionette/test_nfc_error_messages.js @@ +78,5 @@ > > +/** > + * Enables nfc and RE0, registers tech-discovered msg handler, once it's > + * fired set tech-lost handler and disables nfc. In both handlers checks > + * if error message is not present. nit. trailing ws. @@ +172,5 @@ > > +function setTechDiscoveredHandler() { > + let deferred = Promise.defer(); > + > + var techDiscoveredHandler = function(msg) { s/var/let/ @@ +189,5 @@ > + > +function setAndFireTechLostHandler() { > + let deferred = Promise.defer(); > + > + var techLostHandler = function(msg) { ditto @@ +200,5 @@ > + }; > + > + window.navigator.mozSetMessageHandler('nfc-manager-tech-lost', > + techLostHandler); > + toggleNFC(false); This should be done outside, not inside this function.
Attachment #8436898 - Flags: review?(allstars.chh) → review+
Attachment #8436899 - Flags: review?(allstars.chh) → review+
Fixed s/var/let, removed white space. As agreed on IRC left toggleNFC(false), should be refactored in followup bug when we will have support for tech-lost triggering in emulator (Bug 1023079).
Submitting once more to obsolete previous patch.
Attachment #8436898 - Attachment is obsolete: true
Attachment #8437521 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #12) > Submitting once more to obsolete previous patch. This can be done retrospectively, without needing to upload another patch, by going to the details page (eg https://bugzilla.mozilla.org/attachment.cgi?id=8437523&action=edit) -> "edit details" -> check the "obsolete" input field.
Backed out for timeouts/crashes/...: 03:04:41 INFO - TEST-START test_nfc_error_messages.js 03:04:45 INFO - /builds/slave/test/build/tests/marionette/tests/dom/nfc/tests/marionette/test_nfc_error_messages.js, runTest (marionette_test.MarionetteJSTestCase) ... ERROR 03:04:47 INFO - START LOG: 03:04:47 ERROR - Error getting log: Connection to Marionette server is lost. Check gecko.log (desktop firefox) or logcat (b2g) for errors. 03:04:47 INFO - END LOG: 03:04:47 INFO - ====================================================================== 03:04:47 INFO - ERROR: None 03:04:47 INFO - ---------------------------------------------------------------------- 03:04:47 INFO - Traceback (most recent call last): 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette_test.py", line 170, in run 03:04:47 INFO - testMethod() 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette_test.py", line 497, in runTest 03:04:47 INFO - filename=os.path.basename(self.jsFile)) 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 1070, in execute_js_script 03:04:47 INFO - line=None) 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/decorators.py", line 35, in _ 03:04:47 INFO - return func(*args, **kwargs) 03:04:47 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 621, in _send_message 03:04:47 INFO - response = self.client.send(message) 03:04:47 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_transport/transport.py", line 100, in send 03:04:47 INFO - response = self.receive() 03:04:47 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette_transport/transport.py", line 57, in receive 03:04:47 INFO - raise IOError(self.connection_lost_msg) 03:04:47 ERROR - TEST-UNEXPECTED-FAIL | test_nfc_error_messages.js | IOError: Connection to Marionette server is lost. Check gecko.log (desktop firefox) or logcat (b2g) for errors. eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=41428664&tree=B2g-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=41433939&tree=B2g-Inbound remote: https://hg.mozilla.org/integration/b2g-inbound/rev/4f11ff882c7c remote: https://hg.mozilla.org/integration/b2g-inbound/rev/f23a77c2ddb1
This the same error which i'm getting in all nfc tests on latest gecko, after repo sync (Bug 1021180). Nobody else was reporting this. On gecko f297dd29f28bb736dfcfd374dfc62c02ed4f871c everything works fine. Will try to land this once 1021180 is solved.
Depends on: 1021180
One remark here. In the detailed log, it's visible that all the other NFC tests are being skipped because there is no NFC hardware on the device.
blocking-b2g: --- → backlog
test_nfc_error_messages.js was failing because of seg fault in MozNDEFRecord. More details can be seen here https://bugzilla.mozilla.org/show_bug.cgi?id=1021180#c14. The problem with MozNDEFRecord is being solved in Bug 1020034. Once it lands I'll retest this once more.
Depends on: 1020034
Bug 1020034 has landed. I've retested everything and it works fine. Added one small thing in the patch which was pointed out by Thomas. First argument in MozNDEFRecord constructor is not Uint8Array: >-const NDEF_MESSAGE = [new MozNDEFRecord(new Uint8Array(0x01), >+const NDEF_MESSAGE = [new MozNDEFRecord(0x01, Apart from that everything is the same.
Attachment #8437523 - Attachment is obsolete: true
Keywords: checkin-needed
[Blocking Requested - why for this release]: Request to uplift this to 2.0 as 2.0 branch also has this problem. There will be an extra property "errorMsg" contained in the system message.
blocking-b2g: backlog → 2.0?
This patch applies directly to gecko 2.0. I'm doing 2.0 flame build to check if everything is fine. After this I'll request the uplift. I was trying to run 2.0 marionette web-api tests but I cannot start them (see error log below). Will rebuild emulator once more if this persists I'll file a separate bug. >Error running mach: > ['marionette-webapi', 'gecko/dom/nfc/tests/marionette/manifest.ini'] >The error occurred in code that was called by the mach command. This is either >a bug in the called code itself or in the way that mach is calling it. >You should consider filing a bug for this issue. >If filing a bug, please include the full output of mach, including this error >message. >The details of the failure are as follows: >ImportError: cannot import name uses_marionette > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/mach_commands.py", line 90, in run_marionette_webapi > testtype=testtype, topsrcdir=self.topsrcdir, address=None) > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/mach_commands.py", line 30, in run_marionette > from marionette.runtests import ( > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/__init__.py", line 5, in <module> > from gestures import smooth_scroll, pinch > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/gestures.py", line 5, in <module> > from marionette import MultiActions, Actions > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/marionette.py", line 29, in <module> > import geckoinstance > File "/Volumes/firefoxos/B2G/gecko/testing/marionette/client/marionette/geckoinstance.py", line 9, in <module> > from mozrunner import Runner > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/__init__.py", line 6, in <module> > from .local import * > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/local.py", line 20, in <module> > from .base import Runner > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/base/__init__.py", line 2, in <module> > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/base/device.py", line 15, in <module> > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/devices/__init__.py", line 5, in <module> > File "/Volumes/firefoxos/B2G/gecko/testing/mozbase/mozrunner/mozrunner/devices/emulator.py", line 20, in <module>>
Tested on flame with 2.0, works ok. As commented above I've filed Bug 1051762 for problems with marionette test.
Comment on attachment 8440710 [details] [diff] [review] Part:1 Not adding errorMsg if status undefined, applied Yoshis suggestions , applied Thomas remark Requesting for uplift as stated in comment 22. Tested on flame with 2.0 build.
Attachment #8440710 - Flags: approval-mozilla-b2g32?
Comment on attachment 8436899 [details] [diff] [review] Part 2: Reenabling missing marionette tests Requesting for uplift as stated in comment 22. Marionette tests enabled, does not affect the user.
Attachment #8436899 - Flags: approval-mozilla-b2g32?
Comment on attachment 8436899 [details] [diff] [review] Part 2: Reenabling missing marionette tests testonly changes approving for uplift and clearing the blocking flag here.
Attachment #8436899 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Attachment #8440710 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
blocking-b2g: 2.0? → ---
Ah, I'm guessing that's bug 1021180. Looks like some deps need uplift here.
Try run with some of the other deps on this bug included. If it's green, I'll go ahead and push it. https://tbpl.mozilla.org/?tree=Try&rev=507608719f53
We could try to uplift Bug 1020034, which solves this. But I think we might run into some more problems, so I would advise not to do this. The error is caused by the usage of MozNDEFRecord constructor in test_nfc_error_messages.js. I can prepare a 2.0 version of the patch which tests the introduced changes without using MozNDEFRecord.
Flags: needinfo?(kmioduszewski)
Attached patch Part 1: b2g32_v2_0 Gecko version (obsolete) — Splinter Review
Problematic MozNDEFRecord usage removed. Cannot run marionette tests locally for v2.0 due to Bug 1051762, same solution works fine for master so this should clear the errors.
Attachment #8471430 - Flags: approval-mozilla-b2g32?
Comment on attachment 8471430 [details] [diff] [review] Part 1: b2g32_v2_0 Gecko version Discussed this with tauzen and we decided to go with the approach from the Try push (which was green) to keep the code uniform across the branches.
Attachment #8471430 - Flags: approval-mozilla-b2g32?
Attachment #8471430 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: