-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
chore(tests): make tests not print out errors #4141
Conversation
There is still one more error being logged in IE8 but I don't have any more time to track it down right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than my one comment
@@ -27,23 +27,19 @@ QUnit.test('can create a html track element with various properties', function(a | |||
const kind = 'chapters'; | |||
const label = 'English'; | |||
const language = 'en'; | |||
const src = 'http://www.example.com'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not want to test the src
property here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice but it ends up causing an XHR which logs an error in IE8 and there isn't a good way to cancelling the track.
Text Tracks have a source is already tested elsewhere. I think not having an extra error logged for a failed XHR is better than testing something that is partially tested elsewhere anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok acceptable
test/unit/tracks/text-tracks.test.js
Outdated
@@ -365,6 +366,12 @@ QUnit.test('removes cuechange event when text track is hidden for emulated track | |||
mode: 'showing' | |||
}); | |||
|
|||
const oldVttjs = window.vttjs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change probably should be removed from this PR in favor of https://github.com/videojs/video.js/pull/4148/files#diff-45ae6eaab623b37642b347fe7707682aR338
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I'll wait for the master version of #4148 to be merged before merging this one so this can be updated with changes. |
No description provided.