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

Windows-based testing #931

Merged
merged 31 commits into from
Apr 1, 2019
Merged

Conversation

Overdrivr
Copy link
Collaborator

@Overdrivr Overdrivr commented Mar 26, 2019

  • If this is a bugfix, I have provided code that clearly demonstrates the problem and that works when used with this PR
  • I have added a test to the test suite, if necessary
  • I have properly documented new or changed features in the documention, or the docstrings
  • I have properly documented unusual changes to the code in the comments around it
  • I have made note of any breaking/backwards incompatible changes

This PR enables CI testing on Windows platform, with minimal changes to the existing codebase. Closes #628.

Also:

  • Simplifies installation/management of dependencies and virtualenvs for developers by using pipenv
  • Removes the need for using miniconda on appveyor/windows to install dependencies. numpy is no longer problematic to install using pip.
  • Updates bash commands in appveyor script for fetching ImageMagick and refactor them to facilitate further changes
  • Temporarily skip a few tests that generate multiple AudioFileClip, as these generate zombie processes and have the side effect of making the entire test suite fail with Invalid Handle when trying to instanciate subprocesses in subsequent tests (Related: Zombie processes after reader.close() #164, ffmpeg_reader is creating zombie processes #833)
  • Stop relying on external data for running tests (fetched from https://github.com/earney/moviepy_media), because this makes it straight impossible to achieve reproducible build. Total media size is 20MB, which is next to nothing. Also, the media download relied on some functionality inside moviepy which is bad if there is a bug in it (which there was on windows, where in some cases downloaded media were corrupted breaking the entire test suite)
  • Removes calls to sys.path.append('tests') in entire test suite as there is no need to modify path variable at runtime like this

@Overdrivr Overdrivr changed the title I628 windows testing Windows-based testing Mar 26, 2019
@Zulko
Copy link
Owner

Zulko commented Mar 26, 2019

Please provide a description/motivation for this PR. What is it about?

@Overdrivr Overdrivr changed the title Windows-based testing [WIP] Windows-based testing Mar 26, 2019
@Overdrivr
Copy link
Collaborator Author

Slowly getting there: https://ci.appveyor.com/project/RmiBges/moviepy/builds/23374435

Test suite is passing (with 4 disabled tests) on python 3.4 x64, failing on versions above

@Overdrivr
Copy link
Collaborator Author

Reproducible and passing build on appveyor: https://ci.appveyor.com/project/RmiBges/moviepy/builds/23390075

@Overdrivr
Copy link
Collaborator Author

@Zulko I need to make a final review, but I'm pretty much done with this PR. It should make maintenance of moviepy on Windows a lot more easier.

Overall the changes are really minor things, except these two:

  • Added pipenv as a way to manage local dependencies and virtualenvs. I use it on a daily basis, and the automated virtual env management alone makes it very valuable. Not everyone likes pipenv though, so I'd like to know if that's ok for you. It does help greatly in simplifying the appveyor script and achieving reproducible builds.
  • Stopped fetching test media from a remote source, because it's not possible to a) ensure the downloaded files won't be corrupted (this happened on appveyor several times) and b) fetching media relied on moviepy itself which is an issue if the download logic has a bug. Overall, it does not change repo size significantly, the media files weight 20MB in total.

Let me know if those changes are ok with you. If they are, could you register moviepy on appveyor to enable automated checks ? Thanks a lot.

@Zulko
Copy link
Owner

Zulko commented Mar 27, 2019

I have registered Moviepy on Appveyor. As you seem to be the only active maintainer right now, feel free to make changes that make your life easier, as long as they don't change the experience of other maintainers or users (if so, please ask first). Here I understand that this is additional tooling, but the rest will all work as before, right?

@Overdrivr
Copy link
Collaborator Author

@Zulko Thanks for doing the registration, however the appveyor check is not showing up here, if I remember correctly it should show up automatically. Have you set it up using OAuth or Github Apps ?

Yes you're correct, pipenv is additional but optional tooling. It should not break anyone's workflow. I'll always keep in mind to ask before any significant change proposal.

@Zulko
Copy link
Owner

Zulko commented Mar 27, 2019

Using Github Apps, and I created a "moviepy" project

Selection_999(325)

@Overdrivr
Copy link
Collaborator Author

Could you check inside the webhook configuration on Github if the box "Pull Request" is enabled for appveyor ? https://help.appveyor.com/discussions/questions/203-auto-run-tests-on-pull-requests#comment_33212404

image

@Zulko
Copy link
Owner

Zulko commented Mar 27, 2019

The interface looks a bit different, this is what it looks like in github:

Selection_999(326)

@Overdrivr
Copy link
Collaborator Author

It's working now :)

@Overdrivr Overdrivr changed the title [WIP] Windows-based testing Windows-based testing Mar 27, 2019
@Zulko
Copy link
Owner

Zulko commented Mar 27, 2019

Great. Thanks for the help ! Do you want to add your name to the maintainers list at the end of the README ?

@Overdrivr
Copy link
Collaborator Author

For sure I'll do it here then :)

@Overdrivr Overdrivr merged commit 282848a into Zulko:master Apr 1, 2019
@keikoro
Copy link
Collaborator

keikoro commented Apr 11, 2019

This made me realise my handle also needs updating... (only drive-through checking in here after weeks of not even having time to look at GH, lol)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Configure Appveyor support
3 participants