Wednesday, 23 January 2013

WebVTT Cue Timings General Unit Test Review

I am splitting these reviews up for one per C++ file. This should make them easier to deal with.

Changed all comments to reflect rules for parsers.

Added checks to make sure the start and end times are correct.

Fixed spelling "Seperator" to "Separator"

Unused vtt files deleted:
  • cue-times/separator/missing_both_spaces_bad.vtt
  • cue-times/separator/missing_separator_bad.vtt

Cue Timings General (ctgenstructure_unittest.cpp)

Changes

  • CueTimingsSeperator1LeftMix
    • Changes test file to actual filename "cuetimings_separator_1_space_left_mix.vtt"
  • CueTimingsSeperator1RightMix
    • Changes test file to actual filename "cuetimings_separator_1_space_right_mix.vtt"
  • CueTimingsSeperatorNTab
    • Changes test file to actual filename "cuetimings_separator_n_tab.vtt"
  • CueTimingsSeperatorNLMix
    • Changes test file to actual filename "cuetimings_separator_n_spaces_left_mix.vtt"
  • CueTimingsSeperatorNRMix
    • Changes test file to actual filename "cuetimings_separator_n_spaces_right_mix.vtt"
  • CueTimingsSeperatorXMix
    • Changes test file to actual filename "cuetimings_separator_n_spaces_tabs_on_both_sides.vtt"

Added

  • CueTimingsSeparatorMissingLeft
    •  Parser does not require whitespace around separator.
    • Added file "cuetimings_separator_0_spaces_left.vtt"
  • CueTimingsSeperatorMissingRight
    •  Parser does not require whitespace around separator.
    • Added file "cuetimings_separator_0_spaces_right.vtt"
  • CueTimingsSeperatorMissingBoth
    •  Parser does not require whitespace around separator.
    • Added file "cuetimings_separator_0_spaces.vtt"
  • CueTimingsArrowMalformed
    • If the first or second line does not contain --> the cue is bad. The parser will discard bad cues.
    • Added file "cuetimings_arrow_bad.vtt"
  • CueTimingsArrowMissing
    • If the first or second line does not contain --> the cue is bad. The parser will discard bad cues.
    • Added file "cuetimings_arrow_missing.vtt"

Failing Test

[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperator1LeftMix
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperator1RightMix
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperatorNSpace
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperatorNTab
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperatorNLMix
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperatorNRMix
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperatorXMix
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeparatorMissingLeft
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeparatorMissingRight
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsSeperatorMissingBoth
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsArrowMalformed
[  FAILED  ] CueTimingsGeneralStructure.CueTimingsArrowMissing


1 comment:

  1. http://dev.w3.org/html5/webvtt/#webvtt-cue-timings

    According to the syntax, it is actually not correct to not have whitespace around the cuetime separator, hence we report an error. When the spec changes, this behaviour can change.

    If the separator is not found, we actually consider that line as the cue ID. However, if the line following the cue-ID does not contain cuetimes, we consider it an error (WEBVTT_INCOMPLETE, iirc, but don't quote me on that)

    Currently, if either timestamp are malformed, the cue is skipped. This is because the cuetimes cannot be considered an identifier, and do not contain valid values. The second cue is not treated differently from the first in this regard.

    Renaming tests or test files is not really review. Something better would be justifying those decisions and having someone else address those concerns if they are valid.

    ReplyDelete