Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure tests #8

Closed
wants to merge 3 commits into from
Closed

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Jul 11, 2013

What do you think about a layout like this. Each .vtt file has a .js file with the same name. It uses the common util harness module, and can do various things:

  • Use the WebVTTParser object directly -- util.WebVTTParser
  • Get access to the TAP/assertion objects -- util.test
  • Run a set of assertions on a parsed VTT file, reading the whole file in one shot -- util.parseWholeTest(filename, assertionFunc)
  • Run a set of assertions on a parsed VTT file, reading the file in chunks (streaming) -- util.parseStreamingTest(filename, assertionFunc)
  • Do both of the above in order -- util.parseTest(filename, assertionFunc)

The only down side here is how noisy it is to run tests, since you end up running your assertions a lot. Having said that, you get better coverage across different chunk sizes (I'm currently failing 4 of 205 assertions in simple.js, which might be my harness code, or might be a parser bug?).

Let me know how people want to work, and I'll modify things accordingly. Do we care about supporting parallel jsshell runs? I don't, but you might disagree.

@humphd
Copy link
Contributor Author

humphd commented Jul 12, 2013

Andreas, I notice that in the streaming cases where I fail, it's due to me double-counting cues when they are reported early. Your API docs don't mention it, in vtt.js I notice that you have:

      while (self.buffer) {
    // We can't parse a line until we have the full line.                                                                                     
        if (!/[\r\n]/.test(self.buffer)) {
          // If we are in the midst of parsing a cue, report it early. We will report it                                                          
          // again when updates come in.                                                                                                          
          if (self.state === "CUETEXT" && self.cue && self.oncue)
            self.oncue(self.cue);
          return this;
        }

        line = collectNextLine();
...

When I get chunk sizes that are toward the end of the file (e.g., n=36 in the case of simple.vtt), it's double-reporting the single cue, due to this early test. Is that expected? I don't think the spec requires this, but you might have picked up on something I've missed. Here's the output of what I see between the two cases:

Parse Whole File:

oncue - inside CUETEXT section 
{ id: 'ID',
  settings: 
   { region: '',
     vertical: '',
     line: 'auto',
     position: '50%',
     size: '100%',
     align: 'middle' },
  startTime: 0,
  endTime: 2,
  content: 'Text' }
TAP version 13
# simple.vtt
ok 1 should be equal
ok 2 should be equal
ok 3 should be equal
ok 4 should be equal
ok 5 should be equal

1..5
# tests 5
# pass  5

# ok

And when it fails with streaming:

Parse in 2 chunks (n=36)


oncue - early reporting 
{ id: 'ID',
  settings: 
   { region: '',
     vertical: '',
     line: 'auto',
     position: '50%',
     size: '100%',
     align: 'middle' },
  startTime: 0,
  endTime: 2,
  content: '' }
oncue - inside CUETEXT section
{ id: 'ID',
  settings: 
   { region: '',
     vertical: '',
     line: 'auto',
     position: '50%',
     size: '100%',
     align: 'middle' },
  startTime: 0,
  endTime: 2,
  content: 'Text' }
TAP version 13
# simple.vtt
not ok 1 should be equal
  ---
    operator: equal
    expected: 1
    actual:   2
    at: Test._cb (/Users/dave/Sites/repos/vtt.js/tests/util/index.js:15:5)
  ...
ok 2 should be equal
ok 3 should be equal
ok 4 should be equal
ok 5 should be equal

1..5
# tests 5
# pass  4
# fail  1

@humphd
Copy link
Contributor Author

humphd commented Jul 12, 2013

Re-reading things, I do see that you've mentioned that oncue can get called more than once with the same cue. I also see in the spec that it says, "A WebVTT cue identifier can be used to reference a specific cue, for example from script or CSS." So I guess I can key on cue.id internally (i.e., that they will be unique) to make sure I don't add them twice.

@RickEyre
Copy link
Contributor

I haven't seen anything in the WebVTT spec that says that the IDs have to be unique so it could cause problems, but since this is our test harness we can control that.

@humphd
Copy link
Contributor Author

humphd commented Jul 12, 2013

True, and worse still, there's nothing saying you have to have an ID at all on a cue. Maybe I just need something on the cue that indicates if it's partial or complete. What was your thinking here, Andreas?

@RickEyre RickEyre mentioned this pull request Jul 12, 2013
@andreasgal
Copy link
Contributor

I do think we have to do partial/early reporting of cues. When doing document.write-style streaming parsing cues can grow after the fact, so we need a way to update them. I am not sure what a good API is. We can synthesize per-file unique ids for cues but that might be awkward to consume on the DOM construction side. Handing out a complete list every time might be inefficient for large files. Can we get some feedback from whoever is writing the DOM construction on the gecko end? Ralph?

@andreasgal
Copy link
Contributor

I added onpartialcue. oncue is now only invoked once the cue was fully parsed. This should solve your problem right?

@andreasgal
Copy link
Contributor

Want to see if you can make this work now and then feel free to land it. I will parse cue content next, for that we definitely need the json comparison code that builds on top of this.

@humphd
Copy link
Contributor Author

humphd commented Jul 12, 2013

Yeah, onpartialcue will solve my issue. I'm offline til Monday, when I'll pick this up again.

Rick, what about just doing Object literals in the JS vs needing to write separate JSON files? I think we're going to want to be able to compare a whole queue sometimes, and assert on bits inside it (i.e., have both ways available). So making it possible to do both from within the JS and not demanding that you load a json file might be more flexible:

var util = require("./util");

util.parseTest("simple.vtt", function(vtt, t) {
  t.cueEqual(vtt.cues[0],
  {
    id: "",
    settings: {
      region: "",
      vertical: "",
      line: "auto",
      position: "50%",
      size: "100%",
      align: "middle"
    },
    startTime: 0,
    endTime: 2,
    content: "Text"
  });
});

@RickEyre
Copy link
Contributor

I'm thinking that would be good too. More flexibility is definitely better IMO.

What I'm thinking would be good to do is kind of like how Gecko does reftests. We can have a a test.list file or somesuch in any number of the test directories that would have the form of:

basic.vtt basic.json (do a deep equals on vtt file)
complex.vtt complex.js (use custom assertions in complex.js)

include("subdir/test.list")

We can then implement a crawler that will go through each of these and depending on the file extension of the second argument in each line it will do something different. For JSON files it would do a cookie cutter deep equals and for js files it would pass in the custom assertions found in the js to tape.

@RickEyre RickEyre closed this Jul 12, 2013
@RickEyre
Copy link
Contributor

Sorry about that. Slip of the finger!

@RickEyre
Copy link
Contributor

Rebased and landed this in #9.

@RickEyre RickEyre closed this Jul 13, 2013
arski pushed a commit to arski/vtt.js that referenced this pull request Dec 14, 2017
Not having the complete array in the source but instead, just the
possible ranges has a size saving of about 15Kb.
In addition, usage was change to a function called 'isStrongRTLChar'
which makes the code clearer and more readable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants