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

Devrel 332 refactor tests #37

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kylenstone
Copy link
Contributor

@kylenstone kylenstone commented May 18, 2020

My first PR at Frame.io so I'm saving this as a Draft for now. Few things to point out:

  • Removed the run_test() function at the end of integration.py (I also moved this file) because pytest will run the tests automatically. @jhodges10 it's possible this was actually needed for CircleCI so I wanted your eyes on it.
  • I rewrote the frameioclient fixture as setup_client which seems like a better fixture name for actual use in following tests. I modified the other tests to use it.
  • The new test I added specifies a new file upload path single_file_upload, I didn't want to re-use the upload path from the existing test because it was doing so much work already.
    In a later PR, we should figure out a better setup and teardown lifecycle
  • All of this should be 2.7 compliant but please double-check me on that.

@kylenstone kylenstone marked this pull request as ready for review May 18, 2020 03:50
@jhodges10
Copy link
Contributor

If merged, this would definitely break the way CircleCI is setup to test currently so once you get write access to the repo - we should talk through it and get you setup on CircleCI.

As for 2.7 compliance - that's the beauty of the automated tests that run across multiple python versions! If it doesn't pass, you'll get a warning and it won't be allowed to merge.

@kylenstone kylenstone marked this pull request as draft May 18, 2020 15:25
@kylenstone
Copy link
Contributor Author

Thanks for the feedback, back to drafts until sorted

@jhodges10
Copy link
Contributor

@kylenstone one more thing that needs to be updated is the Circle CI config so that it can find the newly re-named integration test file.

@jhodges10
Copy link
Contributor

@kylenstone, now that I've got the integration test in a good place w/ xxhash verification working correctly and passing most of the time - I'd love to come back to this PR and get your improvements merged in!

@subsetpark subsetpark removed their request for review January 26, 2021 14:26
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.

2 participants