FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Devrel 332 refactor tests by kylenstone · Pull Request #37 · Frameio/python-frameio-client · GitHub

/ python-frameio-client Public

Devrel 332 refactor tests - #37

Draft
kylenstone wants to merge 6 commits into
Frameio:masterfrom
kylenstone:DEVREL-332_refactor_tests
ConversationCommits (6)ChecksFiles changed
Draft

Devrel 332 refactor tests#37
kylenstone wants to merge 6 commits into
Frameio:masterfrom
kylenstone:DEVREL-332_refactor_tests

Conversation

kylenstone commented May 18, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 marked this pull request as ready for review May 18, 2020 03:50

jhodges10 commented May 18, 2020

Copy link
Copy Markdown
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.

jhodges10 reviewed May 18, 2020
View reviewed changes
Comment thread tests/test_integration.py
kylenstone marked this pull request as draft May 18, 2020 15:25

kylenstone commented May 18, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, back to drafts until sorted

jhodges10 commented Jun 27, 2020

Copy link
Copy Markdown
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 commented Jul 7, 2020

Copy link
Copy Markdown
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 removed their request for review January 26, 2021 14:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jaypadia-frame Awaiting requested review from jaypadia-frame
1 more reviewer
jhodges10 jhodges10 left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL