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)
Tracking
(firefox31 wontfix, firefox32 wontfix, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)
RESOLVED
FIXED
2.0 S4 (20june)
People
(Reporter: tauzen, Assigned: tauzen)
References
Details
Attachments
(2 files, 7 obsolete files)
864 bytes,
patch
|
allstars.chh
:
review+
bajaj
:
approval-mozilla-b2g32+
|
Details | Diff | Splinter Review |
5.05 KB,
patch
|
bajaj
:
approval-mozilla-b2g32+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Dimi if it's ok with you, I will reenable the missing marionette tests case, as I will be needing them in this bug.
Comment 2•11 years ago
|
||
No problem, sorry for removing it accidentally before :(
Flags: needinfo?(dlee)
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
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.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8435090 -
Attachment is obsolete: true
Attachment #8436884 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8436886 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 8•10 years ago
|
||
white spaces removed
Attachment #8436884 -
Attachment is obsolete: true
Attachment #8436884 -
Flags: review?(allstars.chh)
Attachment #8436898 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 9•10 years ago
|
||
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+
Depends on: 1023079
Assignee | ||
Comment 11•10 years ago
|
||
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).
Assignee | ||
Comment 12•10 years ago
|
||
Submitting once more to obsolete previous patch.
Attachment #8436898 -
Attachment is obsolete: true
Attachment #8437521 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/3e7908638c2a
https://hg.mozilla.org/integration/b2g-inbound/rev/9d7ae79baf16
Keywords: checkin-needed
Comment 14•10 years ago
|
||
(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.
Comment 15•10 years ago
|
||
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
Assignee | ||
Comment 16•10 years ago
|
||
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
Assignee | ||
Comment 17•10 years ago
|
||
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.
Blocks: 973133
Updated•10 years ago
|
blocking-b2g: --- → backlog
Assignee | ||
Comment 18•10 years ago
|
||
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
Assignee | ||
Comment 19•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/6e455ae7ac2f
https://hg.mozilla.org/integration/b2g-inbound/rev/716bf10db437
Flags: in-testsuite+
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6e455ae7ac2f
https://hg.mozilla.org/mozilla-central/rev/716bf10db437
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
[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?
Assignee | ||
Comment 23•10 years ago
|
||
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>>
Assignee | ||
Comment 24•10 years ago
|
||
Tested on flame with 2.0, works ok. As commented above I've filed Bug 1051762 for problems with marionette test.
Assignee | ||
Comment 25•10 years ago
|
||
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?
Assignee | ||
Comment 26•10 years ago
|
||
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 27•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8440710 -
Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Updated•10 years ago
|
blocking-b2g: 2.0? → ---
Comment 28•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/89003c7eb4c4
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f5f52be48ea4
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → wontfix
status-firefox32:
--- → wontfix
status-firefox33:
--- → fixed
Target Milestone: --- → 2.0 S4 (20june)
Comment 29•10 years ago
|
||
Backed out from b2g32 for marionette-webapi permafail.
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6495a7bd61ed
https://tbpl.mozilla.org/php/getParsedLog.php?id=45701741&tree=Mozilla-B2g32-v2.0
Comment 30•10 years ago
|
||
Ah, I'm guessing that's bug 1021180. Looks like some deps need uplift here.
Comment 31•10 years ago
|
||
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
Assignee | ||
Comment 32•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(kmioduszewski)
Assignee | ||
Comment 33•10 years ago
|
||
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 34•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8471430 -
Attachment is obsolete: true
Comment 35•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e7b49fdb07d6
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f100dd10a6f3
Keywords: branch-patch-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•