Closed Bug 997576 Opened 6 years ago Closed 5 years ago

[NFC] Testcase for reading empty NFC tag

Categories

(Firefox OS Graveyard :: NFC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S6 (18july)
tracking-b2g backlog

People

(Reporter: dimi, Assigned: dimi)

References

Details

(Whiteboard: [p=2])

Attachments

(2 files, 4 obsolete files)

Write a test case for empty NFC tag because of bug 997094
Please this into backlog.
blocking-b2g: --- → backlog
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S1 (9may)
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
defer to sprint3.
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Target Milestone: 2.0 S3 (6june) → 2.0 S6 (18july)
Attached patch Add tag clear command (obsolete) — Splinter Review
Attachment #8450797 - Flags: review?(tzimmermann)
Attachment #8450797 - Attachment is obsolete: true
Attachment #8450797 - Flags: review?(tzimmermann)
Attached patch NFC add tag clear command v1 (obsolete) — Splinter Review
Attachment #8450816 - Flags: review?(tzimmermann)
Attached patch Testcase v1 (obsolete) — Splinter Review
Attachment #8450819 - Flags: review?(tzimmermann)
Comment on attachment 8450816 [details] [diff] [review]
NFC add tag clear command v1

Review of attachment 8450816 [details] [diff] [review]:
-----------------------------------------------------------------

This looks very good already. Thanks for the patch.

::: hw/nfc-tag.c
@@ +79,5 @@
>  {
>      ssize_t offset = 0;
>      uint8_t* data;
>  
>      assert(tag);

Please use 'assert(ndef_msg || !len);'. This tests for either an NDEF message, or no message.

@@ +91,5 @@
>  
>      data[offset++] = NDEF_MESSAGE_TLV;
>      data[offset++] = len;
>  
> +    if (len) {

I'm not sure, but I think you can just use '0' as length argument for memcpy.

@@ +92,5 @@
>      data[offset++] = NDEF_MESSAGE_TLV;
>      data[offset++] = len;
>  
> +    if (len) {
> +        assert(ndef_msg);

Should be removed with comment above addressed.

@@ +103,5 @@
>      memset(data + offset, 0, sizeof(tag->t.t1.format.data) - offset);
>  }
>  
>  static void
>  set_t2t_data(struct nfc_tag* tag, const uint8_t* ndef_msg, ssize_t len)

See my comments for |set_t1t_data|.

@@ +128,5 @@
>      memset(data + offset, 0, sizeof(tag->t.t2.format.data) - offset);
>  }
>  
>  static void
>  set_t3t_data(struct nfc_tag* tag, const uint8_t* ndef_msg, ssize_t len)

See my comments for |set_t1t_data|.

@@ +158,4 @@
>  }
>  
>  static void
>  set_t4t_data(struct nfc_tag* tag, const uint8_t* ndef_msg, ssize_t len)

See my comments for |set_t1t_data|.
Attachment #8450816 - Flags: review?(tzimmermann) → review+
Comment on attachment 8450819 [details] [diff] [review]
Testcase v1

Review of attachment 8450819 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Thank you.

::: dom/nfc/tests/marionette/test_nfc_read_tag.js
@@ +9,5 @@
> +// TODO : Get this from emulator console command.
> +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 to head.js in another cleanup bug? There could also be P2P_RE_INDEX_*.

@@ +103,5 @@
> +    testUrlT1TDiscover,
> +    testUrlT2TDiscover,
> +    testUrlT3TDiscover,
> +    testUrlT4TDiscover
> +];

Would it make sense to first run the Url test and afterwards run the Empty tests? This way, the 'clear' operation would really *clear* an NDEF message from the tag, and not just set an empty message.
Attachment #8450819 - Flags: review?(tzimmermann) → review+
Attachment #8450816 - Attachment is obsolete: true
Attachment #8450866 - Flags: review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> Comment on attachment 8450819 [details] [diff] [review]
> Testcase v1
> 
> Review of attachment 8450819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. Thank you.
> 
> ::: dom/nfc/tests/marionette/test_nfc_read_tag.js
> @@ +9,5 @@
> > +// TODO : Get this from emulator console command.
> > +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 to head.js in another cleanup
> bug? There could also be P2P_RE_INDEX_*.
> 
I will create another cleanup bug for it.
Attached patch Testcase v2 (obsolete) — Splinter Review
Attachment #8452102 - Flags: review+
Attachment #8450819 - Attachment is obsolete: true
Attached patch Testcase v3Splinter Review
Address Thomas's comment
Attachment #8452102 - Attachment is obsolete: true
Attachment #8452104 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/861a8e0774f3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.