-
Notifications
You must be signed in to change notification settings - Fork 41
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
Fix MapRecorderWin unit tests #406
Conversation
@@ -47,7 +47,7 @@ describe('maprecorder/MapRecorderWin.vue', () => { | |||
expect(vm.error).to.equal(false); | |||
// Supported codecs under chrome. | |||
expect(vm.mimeType).to.equal('video/webm'); | |||
expect(vm.mimeTypes.length).to.equal(2); | |||
expect(vm.mimeTypes.length).to.equal(3); |
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 was wondering whether this line should stay or be completely removed... the test is about default values... from a point of view, it is assigned when building the data object but on the other, it's value is computed and dependent of the browser the test is running on... so is it really a default value ?
I let you take the decision...
expect(vm.mimeTypes.length).to.equal(3); |
@@ -65,9 +65,11 @@ describe('maprecorder/MapRecorderWin.vue', () => { | |||
|
|||
it('correct supported mime types under chrome', () => { | |||
const mimeTypes = vm.getSupportedMimeTypes(); | |||
expect(mimeTypes.length).to.equal(2); | |||
console.log('MIM', mimeTypes); |
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.
Debugging trace left here ?
console.log('MIM', mimeTypes); |
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.
Sure 🙈 , I'll correct it.
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.
Great ! As you stated, this change has started to make all test runs to fail and I was also going to post a PR to correct this as I encountered the same problem in #405...
I was just thinking whether the test on default Chrome media possibilities was really useful written as is... it is run whatever the browser which is used to run the tests and so doesn't always give the expected result... and even with Chrome, it is only relevant if using the correct version of the browser... Perhaps it would be good to open a separate issue to discuss this and see how to address it... I'd personally write a test to see if at least one format was available on the browser instead of testing a fixed list but it's only a suggestion... I can do it if everybody agrees on it...
For now, this PR fixes the reported problem and should be merged for sure !
Thanks @chrismayer to have done it so fast !
45ba6c7
to
6557de0
Compare
I was thinking the same when analyzing the code for the fix. I'd gladly want to discuss this with you and the others. So I created an issue #407 for that, which will be the base for a possible adaption of the test. Goal of this PR was to get the test suite green again. So, I am going to merge now. |
This fixes the unit tests for the
MapRecorderWin
. Reason is that there is an addition in the mimeType list of the browser. Additionallyvideo/mp4
is listed there now.