1055959 - [NFC] testNfcBadSessionIdError testcase fail

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [ooo 16.04 - 21.04］]

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.

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WIP Patch

Comment 2

[Thomas Nguyen (:tnguyen)]

Attachment: Qemu PR v1

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

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8687057 [details] [review]
Qemu PR v1

Comments addressed in PR, please ask for feedback again after comments answered, thanks!

Comment 4

[Thomas Nguyen (:tnguyen)]

Attachment: external qemu PR

- Removed unused code
- Code readability

Comment 5

[Thomas Nguyen (:tnguyen)]

Attachment: 0001-Bug-1055959-NFC-testNfcBadSessionIdError-testcase-fa.patch

Enable nfc test case

Comment 6

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8687752 [details] [review]
external qemu PR

Looks good to me, thanks!

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 8

[Thomas Nguyen (:tnguyen)]

Attachment: external qemu v2

Thanks for your review.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: external qemu v3

- Fixed nits
Thanks Thomas for your review.

Comment 11

[Thomas Nguyen (:tnguyen)]

Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee8de98a10e

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 14

[Thomas Nguyen (:tnguyen)]

Try result looks ok. Seems that the test cases were not run.
I will do as you said and run try again.
Thanks

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 16

[Thomas Nguyen (:tnguyen)]

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.

Comment 17

[Thomas Nguyen (:tnguyen)]

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.

Comment 18

[Thomas Nguyen (:tnguyen)]

Add checkin-needed

Comment 19

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/platform_external_qemu/commit/b2f83825411be614e8f7ec75fc731fc9c67a7078 for the gaia checkin

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/8703ad09f207

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8703ad09f207