Closed Bug 903030 Opened 6 years ago Closed 6 years ago

[webvtt] Add tests for TextTrackCue validation code

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: reyre, Assigned: reyre)

References

Details

Attachments

(1 file, 5 obsolete files)

We need to have tests that test whether or not the TextTrackCue is validating it's data correctly.
Blocks: 882700
Depends on: 882743
Assignee: nobody → mv.nsaad
Attached patch WIP 01 (obsolete) — Splinter Review
This WIP only addresses the changes made on bug903425. Future patches will include validation for bug882299.

-Added a new TextTrackCue
-Validated enum ranged values such as "lr", "rl", "".
-Validated that values outside of the enum range aren't accepted.
Attachment #803774 - Flags: feedback?(rick.eyre)
Comment on attachment 803774 [details] [diff] [review]
WIP 01

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

Other then that looks good :)!

::: content/media/test/test_texttrackcue.html
@@ +81,5 @@
>        is(cue.endTime, 4, "Cue's end time should be 4.");
>        is(cue.text, "foo", "Cue's text should be foo.");
>  
> +      // bug903425 - Validating DirectionSetting enum
> +      var ttcue = new TextTrackCue(5, 6, "Validating Vertical attribute.");

Move this up and use 'cue' instead of creating a new cue. It should work the same.

@@ +90,5 @@
> +      ttcue.vertical = "rl";
> +      is(ttcue.vertical, "rl", "TextTrackCue.Vertical should be rl (right to left).");
> +      ttcue.vertical = "NonEnumValue";
> +      isnot(ttcue.vertical, "NonEnumValue", "Values outside of the enum shouldn't be accepted.");
> +      is(ttcue.vertical, "rl", "After an invalid value, the last valid value for TextTrackCue.Vertical should be kept.");

We can just check that ttcue.vertical is "rl" instead of checking that it's not "NonEnumValue" and then checking if it's "rl".
Attachment #803774 - Flags: feedback?(rick.eyre) → feedback+
> Comment on attachment 803774 [details] [diff] [review]
> WIP 01
> 
> Review of attachment 803774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We can just check that ttcue.vertical is "rl" instead of checking that it's
> not "NonEnumValue" and then checking if it's "rl".

By doing what you suggested, we wouldn't be testing if values that are outside of the enum are being ignored.
(In reply to Marcus Saad (:msaad) from comment #3)

> By doing what you suggested, we wouldn't be testing if values that are
> outside of the enum are being ignored.

We still would be. Instead of checking if first that align != "NonEnumValue" and then checking that align == "rl" we can just check that it's still "rl". The fact that it's still "rl" means that it's not "NonEnumValue" and that everything worked the way it was supposed too. so:

ttcue.align = "rl";
is(ttcue.align, "rl");
ttcue.align = "NonEnumValue";
is(ttcue.align, "rl", "Should still be rl after setting bogus value.");
I misunderstood you, will be posting an update soon.
Addressed Reyre's previous comments.
Attachment #803774 - Attachment is obsolete: true
Attachment #804127 - Attachment filename: 0001-Bug903030-Adding-Validation-tests-for-TextTrackCue.patch → WIP 02
Attached patch WIP 03 (obsolete) — Splinter Review
Fixed up the patch and addressed the changes suggested.
Attachment #804127 - Attachment is obsolete: true
Attachment #804136 - Flags: feedback?(rick.eyre)
Comment on attachment 804136 [details] [diff] [review]
WIP 03

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

::: content/media/test/test_texttrackcue.html
@@ -84,4 @@
>        is(cue.startTime, 3.999, "Cue's start time should be 3.999.");
>        is(cue.endTime, 4, "Cue's end time should be 4.");
>        is(cue.text, "foo", "Cue's text should be foo.");
> -

Keep this empty line here please.
Attachment #804136 - Flags: feedback?(rick.eyre) → feedback+
Attached patch WIP 04 (obsolete) — Splinter Review
Fixed the space that was accidentally removed.
Attachment #804136 - Attachment is obsolete: true
Attached patch WIP 5 (obsolete) — Splinter Review
Rick,

I've added validation for bug882299 too. Spec is a little messed up as we all know, but I did my best to understand the line algorithm. It depends on the values that snapToLines are set.

Thanks for your feedback!
Attachment #805277 - Attachment is obsolete: true
Attachment #8347655 - Flags: feedback?(rick.eyre)
Comment on attachment 8347655 [details] [diff] [review]
WIP 5

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

Looking good do far, but we also need to test position, size, align, and getCueAsHTML() as well.

::: content/media/test/test_texttrackcue.html
@@ +58,5 @@
> +      is(cue.vertical, "rl", "VTTCue.Vertical should be rl (right to left).");
> +      cue.vertical = "NonEnumValue";
> +      is(cue.vertical, "rl", "Should still be rl after setting arbitrary value.");
> +      // bug882299 - Validating Long or Autokeyword union of TextTrackCue.Line
> +      is(cue.line, "auto", "VTTCue.line should be initiated with autokeyword");

I think we shouldn't add the line tests until bug 882299 lands as line won't even work until then. I can take care of landing those tests when I get the patch ready. I can do snapToLines at that time too as there closely related.
Attachment #8347655 - Flags: feedback?(rick.eyre) → feedback-
Marcus, do you still have time to do this? If not I can take it from you if you don't mind.
I'm going to work on this since I haven't heard back from Marcus in a while and we need to get some more test coverage since the pref is being turned on soon.
Assignee: mv.nsaad → rick.eyre
The code in the test could do with a major refactoring, but I think we should tackle that in bug 983182.
Attachment #8347655 - Attachment is obsolete: true
Attachment #8399596 - Flags: review?(cpearce)
Attachment #8399596 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/c011937283fc
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
The following changeset is now in Firefox Nightly:

> c011937283fc Bug 903030 - Add tests for TextTrackCue validation code. r=cpearce

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
You need to log in before you can comment on or make changes to this bug.