Adds AV Tests and Caption Functionality - #1796
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| return typeGroup === "audio" || typeGroup === "video"; | ||
| } | ||
|
|
||
| // vtt, srt, csv |
There was a problem hiding this comment.
Was unable to find support for csv.
There was a problem hiding this comment.
Have added tests that confirm functionality of both vtt and srt.
| return false; | ||
| } | ||
|
|
||
| const captionTypes = new Set<String>(["text/vtt", "text/srt"]); |
There was a problem hiding this comment.
Did not lose this. It's just higher up in the file now - https://github.com/UniversalViewer/universalviewer/pull/1796/changes#diff-cb64973a8a5a2e0eac4714d6ee79b3016b2f2e7b76e35a803c9f28b4bf197b16R30
|
Thanks, @K8Sewell, this is great! I haven't had a chance to look closely since I'm preparing to leave for my trip and need to choose my battles -- but I'll be happy to look closer when I get back if you want me to. In the meantime, I imagine that @LanieOkorodudu might be a good candidate for reviewing, since she's been more heavily involved in test code lately and probably has some good insights! |
| }); | ||
| }, 60000); | ||
|
|
||
| it("loads the AV manifest into the mediaelement player", async () => { |
There was a problem hiding this comment.
@K8Sewell, Thanks for putting this together, Do you think it's worth adding the canvas title displays "Video Example 3", and that the video can actually play, continue for around 10 seconds, and then pause successfully.
There was a problem hiding this comment.
@LanieOkorodudu thank you so much for this suggestion! I implemented it in the reintegrate AV PR but forgot to add them here as well. Will get that fixed asap and will re-request your review when they are ready. Thanks again!
There was a problem hiding this comment.
I've added a play and pause test to the first AV manifest but to conserve speed of our tests I only included it for that single manifest. If you think I should add it for multiple I can certainly do that. Because the other tests of the other manifests confirm the loading I thought one play/pause expectation was sufficient given the 10 second time expenditure but am happy to defer to what you think is best! Really appreciate your insight!
There was a problem hiding this comment.
Okay we probably leave this one to just play/pause. Thanks for all your work here.
| }); | ||
|
|
||
| // Play/pause button. | ||
| const playButton = await avPage.$( |
There was a problem hiding this comment.
Same here, like play for 10 seconds and and pause to confirm the video is actually play for seconds.
There was a problem hiding this comment.
I wasn't sure with the 10 second time expenditure if it should be included for each manifest so I only included one but will happily update for each to have that expectation if you think that is better.
There was a problem hiding this comment.
I could also shorten the time to 3 seconds or something like that if you would prefer.
There was a problem hiding this comment.
Thanks @K8Sewell. yes you are aright 3 sec is okay to confirm the video actually play.
| }); | ||
| }, 60000); | ||
|
|
||
| it("loads the AV manifest into the AV center panel", async () => { |
There was a problem hiding this comment.
Also confirmed that the title has "Preludio e Coro d'introduzione – Bel conforto al mietitore" as it loaded.
There was a problem hiding this comment.
Thank you for confirming this! I can add an expectation for this as well.
There was a problem hiding this comment.
I ended up adding tests for each manifest's title. Thank you for this suggestion!
|
I really like the approach of creating a separate folder for each component to test. It makes everything much easier to maintain. Thanks! @K8Sewell |
Happy to! Hopefully this is just the start of adding many more helpful tests. :) |
3 participants
Description of what you did:
Related Issue
#1750