Make testing painless #403

joeyw · 2014-01-13T16:54:34Z

State of octokit's tests

Running the tests for octokit is easy and works great currently. Clone down the repo then run script/test and the specs run fine and successfully, as they should.

I think the specs reflect the manner in which octokit is developed. Piece by piece, small feature additions here and there. If you're adding new functionally that requires some repository write access you probably manually create the repo and run then run the tests so the cassettes get recorded and then you don't have to think about it anymore. And it is a complete pain for anyone else trying to fix existing code or add to existing client modules.

To get an idea of how much the tests are not isolated nor do the proper setup and teardown they should be doing you can clone octokit, set the ENV test variables, delete spec/cassettes and run the specs you will likely see results similar to this: 414 examples, 116 failures.

Being able to rerun the tests from scratch isn't the goal here, but rather a byproduct of the primary goal - test isolation.

Introducing isolated test repositories

New helper methods have been added in 466dd99 - setup_test_repo(options={}) as well as a teardown_test_repo(repo). The options hash for setup gets passed to the Octokit.create_repo method. The repository is created with a unique name in the format of api-sandbox-1389737171.566369 and also we make sure it is replaced in the VCR cassettes with <GITHUB_TEST_REPOSITORY>. Using a unique name instead of only api-sandbox is better because:

We don't have to worry about naming or existence conflicts.
We don't care about the temporary repository name.
GitHub hates creating and deleting a repository with the same name in quick succession.

A large portion of the commits in this branch involve converting the specs from the hardcoded api-playground/api-sandbox repository name to a temporary test repository. Lettuce take a look at what that looks like with a glimpse at a before and after excerpt from the Commits Spec. Notice how no setup is done for api-playground/api-sandbox, it is assumed by the test to be already created.

require 'helper'

describe Octokit::Client::Commits do
  before do
    Octokit.reset!
    @client = oauth_client
  end

  describe ".create_commit", :vcr do
    it "creates a commit" do
      last_commit = @client.commits('api-playground/api-sandbox').last
      commit = @client.create_commit("api-playground/api-sandbox", "My commit message", last_commit.commit.tree.sha, last_commit.sha)
      assert_requested :post, github_url("/repos/api-playground/api-sandbox/git/commits")
    end
  end # .create_commit
end

Now consider the spec after we isolate it:

require 'helper'

describe Octokit::Client::Commits do
  before do
    Octokit.reset!
    @client = oauth_client
  end

  context "with test repository" do
    before do
      @test_repo = setup_test_repo(:auto_init => true).full_name
    end

    after do
      teardown_test_repo @test_repo
    end

    describe ".create_commit", :vcr do
      it "creates a commit" do
        last_commit = @client.commits(@test_repo).last
        commit = @client.create_commit(@test_repo, "My commit message", last_commit.commit.tree.sha, last_commit.sha)
        assert_requested :post, github_url("/repos/#{@test_repo}/git/commits")
      end
    end # .create_commit
  end # with test repository
end

These changes are what make up the isolate commits. In some cases there are additional changes which are noted in the commit message. With the help of VCR placeholders tests become isolated to a single test repository. New methods can be added to existing blocks without needing to change anything, just add the code and reference the @test_repo.

Other additions

The ENV variable OCTOKIT_TEST_GITHUB_ORGANIZATION has been added to easily target an organization for testing, it can be referenced by the test helper test_github_org.

Other notable pains and annoyances

Several small changes have been included, see the commit log for more details.

Breaking down this pull request into bite size pieces

coveralls · 2014-01-13T17:04:36Z

Coverage increased (+0.02%) when pulling a9a6228 on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-13T18:32:56Z

Coverage increased (+0.04%) when pulling 1c614be on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-13T23:06:34Z

Coverage increased (+0.02%) when pulling 7fd07ca on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-13T23:22:49Z

Coverage increased (+0.05%) when pulling 1fd96bf on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-13T23:42:14Z

Coverage decreased (-0.12%) when pulling 1fd96bf on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T01:19:44Z

Coverage decreased (-0.12%) when pulling 111d935 on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T01:41:38Z

Coverage increased (+0.05%) when pulling 5449abd on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T01:50:51Z

Coverage increased (+0.02%) when pulling 6b41159 on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T02:42:24Z

Coverage decreased (-0.12%) when pulling 48291f4 on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T12:41:15Z

Coverage increased (+0.02%) when pulling f640789 on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T13:24:33Z

Coverage increased (+0.02%) when pulling d452378 on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T13:43:07Z

Coverage decreased (-0.12%) when pulling 0119589 on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T15:32:17Z

Coverage increased (+0.02%) when pulling b9b481b on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T22:19:07Z

Coverage increased (+0.02%) when pulling f27c44e on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

coveralls · 2014-01-14T22:49:30Z

Coverage increased (+0.05%) when pulling 5bde40f on joeyw:brave-new-tests into 6e0d6cc on octokit:master.

joeyw · 2014-01-20T17:43:43Z

@pengwynn I pushed the rebased branch. I cleaned up the fixes to my changes, revised a few commits and split up a couple commits to make everything nicer but most importantly revised the commit messages to be detailed about the changes made which is how I should have wrote them in the first place.

Conflicts: spec/helper.rb

The api returns utf-8 encoded content but the actual encoding is not set from headers so vcr thinks there is none and force encodes the response body to ASCII-8BIT binary. Instead of messing with our network stack and middleware to give a correct indication of the body encoding this change forces it before the cassette is saved. I rerecorded a cassette included in this commit showing the successful recording of a request with utf-8 characters one of the GitHub user's full name field.

Usable in tests by calling test_github_org helper method.

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.

Makes it easier to setup cassette placeholders by calling use_vcr_placeholder_for(text, replacement) instead of doing all the VCR configure blocks.

Added "with repository" context with test repository setup and teardown. Moved all specs referencing api-playground/api-sandbox into the context and updated references of the repository name to test repository name created in the context setup. Added "with release" context that creates a release in the test repository for the tests that need an exisiting release. Moved the specs requiring a release inside the context and updated the references to the release created in the test setup method. Added "with release asset" context with setup that adds a release asset for the tests that need it. Moved the relevant specs inside the context and updated their references to the asset created in the context setup. Spec results Before changes 13 examples, 0 failures After changes 13 examples, 0 failures

Added "with test repository" context with setup and teardown of test repository. Moved specs referencing api-playground/api-sandbox inside the context and updated the reference to be the test repository created in the context setup. Spec results Before changes 24 examples, 0 failures After changes 24 examples, 0 failures

Added "with repository" context with test repository setup and teardown. Moved .create_status spec into the context and changed api-playground/api-sandbox references to the test repository setup in the context. Spec results Before changes 2 examples, 0 failures After changes 2 examples, 0 failures

Added "with repository" context with test repository setup and teardown. Additionally, a commit comment is created in the context setup to be used by relevant specs. Specs referencing api-playground/api-sandbox moved into the context and their refences updated to the repository setup in the context. Spec results Before changes 6 examples, 0 failures After changes 6 examples, 0 failures

@repo

Changed .create_repository for organization repositories spec to use a test_repo name, changed the spec to create the repository in the before block and teardown in after block. Renamed "methods that require a new @repo" context to "with repository". Changed "with repository" setup to use setup_test_repo instead of "an-repo". Changed github_url call to basic_github_url in "creates a repository" spec because a basic auth client is used by setup_test_repo in the context. Changed specs that add and remove a collaborator to and from a repository for testing from "pengwynn" to "api-padawan". Renamed "methods that need an existing hook" context to "with hook". Changed .delete_repository spec to use basic_auth_client since the oauth_client doesn't have the proper scopes to delete repositories by default. Changed .fork spec to use basic_auth_client when cleaning up the forked repository since oauth_client doesn't have delete scope by default. Expanded ".subscription" spec to test "with a subscription" and "without a subscription". Previously only "with a subscription" was being tested. Spec results Before changes 48 examples, 0 failures After changes 49 examples, 0 failures .subscription spec was expanded to cover another case resulting in an example increase.

Removed "methods that require a ref" context that wrapped all the specs. Broke down specs into "with repository" and "with ref" contexts where appropriate. Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated the references to use the test repository created in the context. Added "with ref" context with setup to create a reference for the relevant specs to test against. Moved relevant specs into the context and updated them to use the ref created in the context setup. Spec results Before changes 8 examples, 0 failures After changes 8 examples, 0 failures

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into context and updated references to use repository setup in the context.

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated the references to use the repository created in the context setup.

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated reference to use repository created in context setup. Added "with milestone" context that sets up milestone to test against. Moved relevant specs and updated references to use milestone created in context setup.

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated specs to use repository created in context setup. Updated .org_issues to use test_github_org helper instead of hardcoded dotfiles organization. Moved .repository_issues_comments, .issue_comment and .issue_comments specs to the top of the spec file for the sole purpose of making the contexts nest nicely and less confusing. Renamed "methods requiring a new issue" context to "with issue". Renamed "methods requiring a new issue comment" context to "with issue comment".

Moved .labels_for_milestone spec to top of file for context nesting clarity. Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated relevant specs to use the repository setup in the context. Renamed "methods requiring a new label" context to "with label". Renamed "methods requiring a new issue" context to "with issue".

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and changed specs to use repository setup in the context. Renamed "methods that need a thread context" context to "with thread".

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated specs to use repository setup in the context.

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated specs to use repository setup in the context. Added "with modified branch" context which sets up a branch other than master with commits to test .create_pull_request_for_issue with ease. Renamed "methods that require a new pull" context to "with pull request". Changed .merge_pull_request spec from a stubbed spec to use a real request and a VCR cassette. Added "with pull request comment" context which sets up a pull request comment for testing. This removes the setup in each of the individual specs .create_pull_request_comment, .create_pull_request_comment_reply, .update_pull_request_comment and .delete_pull_request_comment. Conflicts: spec/cassettes/Octokit_Client_PullRequests/_pull_requests/returns_all_pull_requests.json

Changed specs using api-playground as the target test organization to use test_github_org test helper method. Moved .remove_organization_member and .user_teams specs to top of spec file for context nesting clarity. Renamed "methods that require a new team" context to "with team". Removed the modified ordering option for the context. Added "with organization repository" context with test organization repository setup and teardown. Moved relevant specs into the context and utilize reposistory setup in the context. Added "with team repository" context which sets up a team repository for testing.

There isn't an immediately apparant method to create a notification with a single account. The cost of having these methods not stubbed is manual effort or an additional test account required for them to be rerecordable.

Spec was based on changing data in a real repo. Changed to use paginated search results with 3 fixed results so the test is easily repeatable without changes. Remove unused cassette. Refreshed all cassettes.

Added cleanup to tests to delete tokens created by tests. Filtered tokens to be replaced by <<ACCESS_TOKEN>> to help prevent sensitive data leaking if a token doesn't get deleted by cleanup.

pengwynn mentioned this pull request Jan 20, 2014

Reset Octokit before and after Authorization and Client specs #407

Merged

joeyw added 13 commits February 7, 2014 10:34

Change basic_auth_client test helper to create new client each time.

8c449ae

Conflicts: spec/helper.rb

Add OCTOKIT_TEST_GITHUB_ORGANIZATION test ENV variable.

89a6e61

Usable in tests by calling test_github_org helper method.

Add VCR cassette placeholders for test repositories and org team ids

32d7069

Add use_vcr_placeholder_for test helper method

a9813fa

Makes it easier to setup cassette placeholders by calling use_vcr_placeholder_for(text, replacement) instead of doing all the VCR configure blocks.

Isolate PubSubHubbub specs

542e76f

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into context and updated references to use repository setup in the context.

joeyw added 14 commits February 7, 2014 10:34

Isolate Objects specs

2eb26b3

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated the references to use the repository created in the context setup.

Isolate Objects specs

4d7385e

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated the references to use the repository created in the context setup.

Isolate Contents specs

fe09cc5

Added "with repository" context with test repository setup and teardown. Moved specs referencing api-playground/api-sandbox into the context and updated specs to use repository setup in the context.

Change Notifications specs requiring thread context to stubs.

7191683

There isn't an immediately apparant method to create a notification with a single account. The cost of having these methods not stubbed is manual effort or an additional test account required for them to be rerecordable.

Test pagination against unchanging exact paginated resource.

aa22d01

Spec was based on changing data in a real repo. Changed to use paginated search results with 3 fixed results so the test is easily repeatable without changes. Remove unused cassette. Refreshed all cassettes.

Gists Spec: Delete test gists in after block

1f629b5

Authorizations Spec add cleanup, filter sensitive tokens created.

79eb0cb

Added cleanup to tests to delete tokens created by tests. Filtered tokens to be replaced by <<ACCESS_TOKEN>> to help prevent sensitive data leaking if a token doesn't get deleted by cleanup.

Filter private tokens from Feeds specs on record.

6dc706e

joeyw mentioned this pull request Feb 8, 2014

Add test repository setup and teardown helpers. #429

Closed

joeyw closed this May 9, 2014

Make testing painless #403

Make testing painless #403

Uh oh!

Conversation

joeyw commented Jan 13, 2014

State of octokit's tests

Introducing isolated test repositories

Other additions

Other notable pains and annoyances

Breaking down this pull request into bite size pieces

Uh oh!

coveralls commented Jan 13, 2014

Uh oh!

coveralls commented Jan 13, 2014

Uh oh!

coveralls commented Jan 13, 2014

Uh oh!

coveralls commented Jan 13, 2014

Uh oh!

coveralls commented Jan 13, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

coveralls commented Jan 14, 2014

Uh oh!

joeyw commented Jan 20, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants