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

Tests: Add video samples #23

Closed
wants to merge 3 commits into from
Closed

Conversation

@mkchoi212
Copy link
Contributor

@mkchoi212 mkchoi212 commented Jul 5, 2018

4 sample videos have been downloaded and added.
Each video is about 3-5 seconds for the total size of 1.5 MB.

All videos have been downloaded from https://vimeo.com/user32335610 and are under the Creative Commons Public Domain Dedication license.

After downloading the clips, I shortened the videos and changed the file formats to diversify the sample.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch 5 times, most recently from d279284 to 3f33047 Jul 5, 2018
@fkuehne
Copy link
Contributor

@fkuehne fkuehne commented Jul 8, 2018

Is it possible to store those videos online (we can provide storage space) and download them on-the-fly so we don't need to add so much binary data to the git?

@mkchoi212
Copy link
Contributor Author

@mkchoi212 mkchoi212 commented Jul 9, 2018

Good idea. If you could upload them to a server and provide me a link to them, that would be great!

@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch from 3f33047 to 3e4f31f Jul 9, 2018

# URL and desired file names of four videos to be downloaded
VIDEOS = [
('https://www.sample-videos.com/video/mp4/240/big_buck_bunny_240p_1mb.mp4', 'bird.m4v'),

This comment has been minimized.

@mkchoi212

mkchoi212 Jul 9, 2018
Author Contributor

These links must be updated :D

@carolanitz
Copy link
Member

@carolanitz carolanitz commented Jul 9, 2018

if we upload it somewhere we have the same issues as in the mainrepo which is tests intermittedly failing because of slow connection or failing connection and if the tests are not reliable they're useless

@fkuehne
Copy link
Contributor

@fkuehne fkuehne commented Jul 9, 2018

Then we should have a git submodule for the files. Adding 6MB of binary blobs to the git just for testing will increase download time and used disk space for everyone, so it would be nicer to optionalize this.

@carolanitz
Copy link
Member

@carolanitz carolanitz commented Jul 9, 2018

Can we look into best practices for this usecase? We're not the only ones who deal with this problem so i'd like to know how other projects handle this case

@jbkempf
Copy link
Contributor

@jbkempf jbkempf commented Jul 9, 2018

Git-LFS is an option, but a pain to synchronize between repos.

The other is a local cache from distant files, like the contrib tarballs (SHA-512) and is probably the simplest.

@carolanitz
Copy link
Member

@carolanitz carolanitz commented Jul 9, 2018

Git-LFS is for large files, we're talking 1,5 MB here and isn't absolutely common to check in files for testing into a repo ?

@carolanitz
Copy link
Member

@carolanitz carolanitz commented Jul 9, 2018

btw don't get me wrong, this is absolutely a valid point that we might unnecessarily enlarge our repository here. I could've sworn though that checking test data into your repository is common practice so I didn't even second guess it.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch 2 times, most recently from e22a8c3 to e02937d Jul 17, 2018

# URL and desired file names of videos to be downloaded
VIDEOS = [
('https://www.sample-videos.com/video/mp4/240/big_buck_bunny_240p_1mb.mp4', 'bird.m4v'),

This comment has been minimized.

@mkchoi212

mkchoi212 Jul 17, 2018
Author Contributor

These links will be replaced by links for code.v.o once available

@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch from e02937d to 0046305 Jul 19, 2018
@carolanitz
Copy link
Member

@carolanitz carolanitz commented Jul 20, 2018

could you rebase this on master ? or does circleci failing because of something else ?

@mkchoi212
Copy link
Contributor Author

@mkchoi212 mkchoi212 commented Jul 22, 2018

The PR for TestAssets hasn’t been merged yet :(

I’ll ping thresh

@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch 3 times, most recently from 555d465 to 5b08a75 Jul 24, 2018
@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch 5 times, most recently from 1c012ef to 0fb0bf3 Jul 25, 2018
@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch from 0fb0bf3 to 814f099 Jul 25, 2018
@mkchoi212
Copy link
Contributor Author

@mkchoi212 mkchoi212 commented Jul 27, 2018

From Twitter, it seems like loading the videos from the test bundle after being downloaded is the best way to go; which is how this PR is setup.

Pros are

  • Tests can't start if the assets can't be downloaded from code.v.o/TestAssets
    • This is good because if we don't load the videos from the bundle, the tests will start and fail with messages like "can't open file x"
  • Buying into Xcode's infra

Cons are

  • Minor complexity during initial test run
  • Additional files added to .pbxproj
@mkchoi212 mkchoi212 force-pushed the mkchoi212:sample-video branch from 814f099 to 2ce1c3c Jul 27, 2018
@carolanitz
Copy link
Member

@carolanitz carolanitz commented Jul 30, 2018

merged with commit aa98dd3 &&
commit 1be28e1 &&
commit 458b355

@carolanitz carolanitz closed this Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.