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

CB-14091: fix tests code for stream url and remove browser #166

Merged
merged 7 commits into from
Jun 22, 2018

Conversation

knight9999
Copy link
Contributor

@knight9999 knight9999 commented May 16, 2018

Platforms affected

Tests code only
Browser (test)
iOS (test)

What does this PR do?

Removing Browser's play audio tests. (which fails)
Change test streaming mp3 file's url in iOS tests.

What testing has been done on this change?

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@knight9999 knight9999 changed the title fix tests code for fix stream url and remove browser CB-14091: fix tests code for fix stream url and remove browser May 16, 2018
@knight9999 knight9999 changed the title CB-14091: fix tests code for fix stream url and remove browser CB-14091: fix tests code for stream url and remove browser May 16, 2018
@shazron
Copy link
Member

shazron commented May 21, 2018

One failure on android-4.4 on Travis. I'm not sure what our support matrix for Android is currently. @infil00p @macdonst ?

@knight9999
Copy link
Contributor Author

@shazron Strange error since I’ve only fixed the iOS portion. Maybe it’s an TravisCI error so could you re-run the test if that’s possible?

@shazron
Copy link
Member

shazron commented May 22, 2018

@knight9999 I'll close and re-open this PR to re-trigger Travis

@shazron shazron closed this May 22, 2018
@shazron shazron reopened this May 22, 2018
@shazron
Copy link
Member

shazron commented May 22, 2018

Looks like master builds have been failing on Android for a while: https://github.com/apache/cordova-plugin-media/commits/master

@shazron
Copy link
Member

shazron commented May 22, 2018

Actually, looks like an npm issue Callback called more than once

@shazron
Copy link
Member

shazron commented May 22, 2018

See npm/npm#9418 (comment)
We need to bump the npm version to the latest npm@2 or npm@3 at least

upgraded node.js to use 6.14.2
Node 4 is deprecated.
@knight9999
Copy link
Contributor Author

It appears that the test version of node_js was still using 4.2 which have already been deprecated in other Cordova repos for example cordova-cli. I have upgraded the Travis CI yaml file to reflect this change and now use version 6.14.2. All tests except for the Android 4.4 are now passing.

@shazron
Copy link
Member

shazron commented Jun 19, 2018

@knight9999
Copy link
Contributor Author

I have removed tests of streaming for Android 4.4.

Remark that I have used one test mp3 file WEB_MP3_STREAM = 'https://cordova-develop.github.io/cordova-plugin-media/res/mozart_serenade4_01.mp3'.
This is because that the old mp3 file WEB_MP3_STREAM = 'http://c22033-l.i.core.cdn.streamfarm.net/22033mdr/live/3087mdr_figaro/ch_classic_128.mp3' is not available now.
If someone can copy the above mp3 file to the official site https://cordova.apache.org, please copy it.

@shazron
Copy link
Member

shazron commented Jun 19, 2018

I'm not sure we have the copyright, any files we put on Apache need to have the right license (Apache 2.0 or one of the approved licenses).

@knight9999 knight9999 closed this Jun 20, 2018
@knight9999 knight9999 reopened this Jun 20, 2018
@knight9999
Copy link
Contributor Author

knight9999 commented Jun 20, 2018

I found the original testing mp3 file ch_classic_128.mp3 from web.archive.org. (https://web.archive.org/web/20170205201205if_/http://c22033-l.i.core.cdn.streamfarm.net/22033mdr/live/3087mdr_figaro/ch_classic_128.mp3)
I put this mp3 file on my repository and used it to test. https://cordova-develop.github.io/cordova-plugin-media/res/ch_classic_128.mp3
I removed previous streaming test mp3 file https://cordova-develop.github.io/cordova-plugin-media/res/mozart_serenade4_01.mp3.

If you can copy the original mp3 file ch_classic_128.mp3 to the cordova official site (For example, https://cordova.apache.org/downloads/), please copy it to use in testing.

@shazron
Copy link
Member

shazron commented Jun 20, 2018

Sorry to be a stickler on this -- just because something is in the Wayback Machine, does not mean that it lacks copyright. Until we can assert what license the file is in (like any code contribution), we can't add it to Apache. For example, BlueZed.mp3 was an original creation by @purplecabbage, a Cordova Committer and PMC member and assigned to Apache. In any case, I'm sure we can find a public domain/Apache-2.0 licensed music sample somewhere.

For example, any of the clips here are public domain: https://musopen.org/

@purplecabbage
Copy link
Contributor

purplecabbage commented Jun 20, 2018

Why you not like song? Is it rock too hard?
jk . I can change the format if that is the issue.

@knight9999
Copy link
Contributor Author

Thanks shazron and purplecabbage,
I have updated the tests code to use BlueZedEx.mp3 file for each test.
Sorry, I can not make song.

@purplecabbage
Copy link
Contributor

my song is really just 8 bars repeating for 60 seconds ... the word 'song' may be a bit of stretch ...

@shazron shazron merged commit 524c337 into apache:master Jun 22, 2018
uaza pushed a commit to quiply/cordova-plugin-media that referenced this pull request Oct 22, 2020
* fix tests code for fix stream url and remove browser
* Update .travis.yml - upgraded node.js to use 6.14.2 (Node 4 is deprecated)
* remove streaming test for Android 4.4 (KitKat does not support modern SSL)
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.

4 participants