-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add test repository setup and teardown helpers. #429
Conversation
Added test_repo test helper method to generate a unique-enough repository name and filter it from VCR cassettes as <GITHUB_TEST_REPOSITORY>. Added setup_test_repo test helper method to create a new repository on GitHub to test against. Method returns the newly created repository. Added teardown_test_repo test helper method to delete a test repository from GitHub. Method takes a full repository name.
Prevent new cassette recordings from spamming GitHub too often with repository creation.
|
||
# Generate test repo name and filter it from VCR | ||
def test_repo | ||
repo = "octokit-test-repo-#{Time.now.to_f.to_s}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be careful here to ensure we're only creating repos when necessary. Anything based on time will always be unique and thrash the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repositories are only created when a cassette is recorded and then there is a 5 second delay after creating it to prevent spamming too fast. If even that is too much I think it would be possible to setup single test repository in a global before
hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If even that is too much I think it would be possible to setup single test repository in a global before hook.
I'd like to investigate that approach. Other than a couple of repository creation methods, the rest of the tests don't care what the repository is, just as long as there is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a global test repo solves the hardcoded test repo names problem but adds the annoyance of having to worry about deleting the additional setup and teardown cassettes it will use. This is more useless knowledge that will have to be documented and read by anyone wanting to contribute and I sense may cause headaches.
By using individual test repos everything is encapsulated in its own single cassette file, completely isolated to the point that it just feels right and that that is how it is supposed to be. There is a little more setup and teardown code but overall it makes things easier for making changes.
👍 |
After removing the hardcoded test repo names I started to question this proposal. I was wrong. I think you are right and a single test repository should be fine. I didn't know how to make the setup nicely and automated the way I wanted which led me the solution in this pull request. I know I promoted this approach heavily but I want to suggest another alternative before going with it. After trying out a few different approaches I think I found a way to lazily create the test repository when it's needed. Before making real requests we check if the test repo exists, create it if it doesn't. This setup exists outside a cassette so we don't have to worry about that. There custom header is stop the hook from sticking in a recursive loop since it makes a real request itself and the hook would keep calling itself. VCR.configure do |c|
c.before_http_request(:real?) do |request|
next if request.headers['X-VCR-SKIP-BEFORE-HOOK']
options = {:headers => {'X-VCR-SKIP-BEFORE-HOOK' => true}}
if !oauth_client.repository?(test_github_repository, options)
opts = options.merge({:auto_init => true})
oauth_client.create_repository test_github_repository, opts
end
end
end What do you think? |
Went with #452 instead. |
Added
test_repo
test helper method to generate a unique-enoughrepository name and filter it from VCR cassettes as
<GITHUB_TEST_REPOSITORY>
.Added
setup_test_repo
test helper method to create a new repository onGitHub to test against. Method returns the newly created repository.
Added
teardown_test_repo
test helper method to delete a test repositoryfrom GitHub. Method takes a full repository name.
These methods will remove the need for hardcoded test repository names which cause headaches for anyone working on existing tests or adding new methods instead of a group of tests that utilize any type of setup in a
before
block. Example usage can be seen in #403 (comment).