Closed Bug 1055959 Opened 7 years ago Closed 6 years ago

[NFC] testNfcBadSessionIdError testcase fail

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
2.6 S1 - 11/20
Tracking Status
firefox45 --- fixed

People

(Reporter: dimi, Assigned: tnguyen)

References

Details

Attachments

(2 files, 4 obsolete files)

Run following command 
./mach marionette-webapi ./gecko/dom/nfc/tests/marionette/manifest.ini

Testcase testNfcBadSessionIdError in test_nfc_error_messages.js will fail.

This is related to Bug 1050621, because the bug fixing change the behavior of nfcd (do not reset rf when receive deactivate notification).
So when emulator deactivate remote end point and try to activate it again, it will fail to activate,

I will fix this bug in emulator side.
Attached patch WIP Patch (obsolete) — Splinter Review
Assignee: dlee → tnguyen
Attached file Qemu PR v1 (obsolete) —
There's something wrong in nci state transition.
Deactivate() called from JS does not trigger state transition, as the result the next activate command will throw an error.
According to NFC Controller Interface (NCI) Specification, NCI state should be changed after NFCC send RF_DEACTIVATE_RSP/NTF. 
Thanks
Attachment #8478925 - Attachment is obsolete: true
Attachment #8687057 - Flags: feedback?(dlee)
Comment on attachment 8687057 [details] [review]
Qemu PR v1

Comments addressed in PR, please ask for feedback again after comments answered, thanks!
Attachment #8687057 - Flags: feedback?(dlee)
Attached file external qemu PR (obsolete) —
- Removed unused code
- Code readability
Attachment #8687057 - Attachment is obsolete: true
Attachment #8687752 - Flags: feedback?(dlee)
Enable nfc test case
Attachment #8687762 - Flags: review?(allstars.chh)
Attachment #8687762 - Flags: review?(allstars.chh) → review+
Comment on attachment 8687752 [details] [review]
external qemu PR

Looks good to me, thanks!
Attachment #8687752 - Flags: feedback?(dlee) → feedback+
Attachment #8687752 - Flags: review?(tzimmermann)
Comment on attachment 8687752 [details] [review]
external qemu PR

Hi!

thanks for the fixing this bug. It's been a while since I worked on this code, so I had to dig through it to understand what the problem is. Let me know if I missed the point somewhere.

I left a number of comments in the pull request. Please ask me for another review once you've gone through them.
Attachment #8687752 - Flags: review?(tzimmermann)
Attached file external qemu v2 (obsolete) —
Thanks for your review.
Attachment #8687752 - Attachment is obsolete: true
Attachment #8688283 - Flags: review?(tzimmermann)
Comment on attachment 8688283 [details] [review]
external qemu v2

Looks good now. Thanks for this fix! Please have a look at the nits I commented in the pull request before landing the patch.
Attachment #8688283 - Flags: review?(tzimmermann) → review+
Attached file external qemu v3
- Fixed nits
Thanks Thomas for your review.
Attachment #8688283 - Attachment is obsolete: true
Attachment #8688345 - Flags: review+
I saw that you closed the old pull requests on Github. Instead, you can also just push the changes into your source branch on Github and the pull request should be updated automatically. Might be more comfortable.
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #11)
> Try result
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee8de98a10e

It's not that easy because try won't pick-up your emulator changes by default. Only Gecko changes will be tested.

To modify one of the other repositories, you have to modify the respective sources.xml file under b2g/config. For example to test the change to emulator-x86-kk, you'd have to change b2g/config/emulator-x86-kk/sources.xml.

You have to add your Github account to the repositories and change the remote and revision of the non-Gecko project to point to your patch.

If you push that change to try, try will fetch your non-Gecko modifications.
Try result looks ok. Seems that the test cases were not run.
I will do as you said and run try again.
Thanks
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #14)
> Try result looks ok. Seems that the test cases were not run.
> I will do as you said and run try again.
> Thanks

I don't think we currently run automated NFC tests.
Thanks Thomas,
Currently, marionette-webapi nfc tests were skipped in testing/marionette/client/marionette/tests/webapi-tests.ini). Also, some job flags would be changed to enable running test case in try. I will try to modify and see

Remove checkin-needed to cherry-pick to other emulator platforms (ics, lollipop, jb, ...) and push to try.
Keywords: checkin-needed
Try (staging) result running marionette-webapi 
https://treeherder.allizom.org/#/jobs?repo=try&revision=bfd326e5ce7d&selectedJob=14334850&filter-tier=1

The test result failed (but it failed in test_cellbroadcast_umts_language_and_body.js, it's another story)
All Nfc test cases passed.
Add checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8703ad09f207
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S4 (12sep) → 2.6 S1 - 11/20
You need to log in before you can comment on or make changes to this bug.