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

Add missing endpoints to Reactions #1637

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Add missing endpoints to Reactions #1637

merged 2 commits into from
Oct 27, 2023

Conversation

wJoenn
Copy link
Contributor

@wJoenn wJoenn commented Oct 6, 2023

Resolves #1633


Before the change?

  • There were no endpoints to get a release's reactions, create a release reaction or delete a release reaction

After the change?

  • I added those endpoints to the Client::Reactions module

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

I'm having an issue trying to run the tests I've written.
I've exported a OCTOKIT_TEST_GITHUB_TOKEN with a new token I've created before forking the app (the token has all the rights I could give it) but despite doing that I get a Octokit::Unauthorized: POST https://api.github.com/user/repos: 401 - Bad credentials error.

I've tried another token that I use in my personal repos and it fails too so I don't think the issue comes from the tokens but probably from something I haven't set up proprelly.

Because of that I haven't been able to assert passing tests and complete coverage yet.

My set up process was to fork the repo then

git clone ...
cd octokit.rb
script/bootstrap
export OCTOKIT_TEST_GITHUB_TOKEN=...
script/test

image

@wJoenn
Copy link
Contributor Author

wJoenn commented Oct 16, 2023

@nickfloyd Maybe you'd have some insights to help me finish this PR ? 🙏

@kfcampbell
Copy link
Member

Are you able to run the tests on the main branch following those steps? When I run ./script/test on main I get the following output:

===> Bundling...
Please export OCTOKIT_TEST_GITHUB_LOGIN
Please export OCTOKIT_TEST_GITHUB_PASSWORD
Please export OCTOKIT_TEST_GITHUB_TOKEN
Please export OCTOKIT_TEST_GITHUB_CLIENT_ID
Please export OCTOKIT_TEST_GITHUB_CLIENT_SECRET
===> Running specs...
rspec-queue

Randomized with seed 20797

Starting test-queue master (/tmp/test_queue_164308_4020.sock)
/home/kfcampbell/github/dev/octokit.rb/spec/octokit/client/codespaces_secrets_spec.rb:6: warning: method redefined; discarding old create_box
/home/kfcampbell/github/dev/octokit.rb/spec/octokit/client/actions_secrets_spec.rb:6: warning: previous definition of create_box was here
/home/kfcampbell/github/dev/octokit.rb/spec/octokit/client/dependabot_secrets_spec.rb:6: warning: method redefined; discarding old create_box
/home/kfcampbell/github/dev/octokit.rb/spec/octokit/client/codespaces_secrets_spec.rb:6: warning: previous definition of create_box was here

==> Summary (8 workers in 9.7157s)

    [ 1]                                      10 examples, 0 failures         2 suites in 2.4977s      (pid 164311 exit 0 )
    [ 2]                                      88 examples, 0 failures         9 suites in 9.7121s      (pid 164312 exit 0 )
    [ 3]                                     110 examples, 0 failures        11 suites in 9.7050s      (pid 164313 exit 0 )
    [ 4]                                     119 examples, 0 failures        12 suites in 9.6922s      (pid 164314 exit 0 )
    [ 5]                                     109 examples, 0 failures         3 suites in 9.6821s      (pid 164315 exit 0 )
    [ 6]                                     159 examples, 0 failures        19 suites in 9.6698s      (pid 164316 exit 0 )
    [ 7]                                     114 examples, 0 failures         5 suites in 9.6565s      (pid 164317 exit 0 )
    [ 8]                                     195 examples, 0 failures        12 suites in 9.6379s      (pid 164318 exit 0 )

On your branch, I see the errors present in CI. There's a code problem to be addressed here before tests are runnable.

@wJoenn
Copy link
Contributor Author

wJoenn commented Oct 24, 2023

Hey @kfcampbell
Yeah I'm able to run the test on main just fine, that's because the cassettes already have all the http responses stored in json files so they don't need to make a new one per test.

In the case of my 3 tests though those cassettes don't exist yet so an actual http request need to be made and that's the issue.
If I were to take this test for example :

describe Octokit::Client::Reactions do
  before do
    Octokit.reset!
    @client = oauth_client
  end
  
  context 'with repository', :vcr do
    before(:each) do
      @repo = @client.create_repository('an-repo', auto_init: true)
    end

    after(:each) do
      @client.delete_repository(@repo.full_name)
    rescue Octokit::NotFound
    end
    
    context 'with release' do
      before do
        @release = @client.create_release(@repo.full_name, 'v1.0.0')
        @reaction = @client.create_release_reaction(@repo.full_name, @release.id, '+1')
      end

      describe '.release_reactions' do
        it 'returns an Array of reactions' do
          reactions = @client.release_reactions(@repo.full_name, @release.id)
          expect(reactions).to be_kind_of Array
          assert_requested :get, github_url("/repos/#{@repo.full_name}/releases/#{@release.id}/reactions")
        end
      end # .release_reactions
    end
  end
end

I get this error

1) Octokit::Client::Reactions with repository with release .delete_release_reaction deletes the reaction
     Got 0 failures and 2 other errors:

     1.1) Failure/Error: raise error

          Octokit::Unauthorized:
            POST https://api.github.com/user/repos: 401 - Bad credentials // See: https://docs.github.com/rest
          # ./lib/octokit/response/raise_error.rb:14:in `on_complete'
          # ./lib/octokit/middleware/follow_redirects.rb:73:in `perform_with_redirection'
          # ./lib/octokit/middleware/follow_redirects.rb:61:in `call'
          # ./lib/octokit/connection.rb:156:in `request'
          # ./lib/octokit/connection.rb:28:in `post'
          # ./lib/octokit/client/repositories.rb:160:in `create_repository'
          # ./spec/octokit/client/reactions_spec.rb:11:in `block (3 levels) in <top (required)>'

     1.2) Failure/Error: @client.delete_repository(@repo.full_name)

          NoMethodError:
            undefined method `full_name' for nil:NilClass
          # ./spec/octokit/client/reactions_spec.rb:15:in `block (3 levels) in <top (required)>'

So the issue is not coming from my code (yet), my code has not even be executed by the test because I'm unable to create the @repo despite doing export OCTOKIT_TEST_GITHUB_TOKEN=<<token>> or export OCTOKIT_TEST_GITHUB_TOKEN=Bearer <<token>> in my terminal before running the tests

@kfcampbell
Copy link
Member

I've spent a little while longer looking at this. I'm able to regenerate the spec file by setting the environment variable OCTOKIT_TEST_VCR_RECORD to true and then running bundle exec rspec spec/octokit/client/reactions_spec.rb:148.

Doing this gives me some failure output:

Failures:

  1) Octokit::Client::Reactions with repository with release .delete_release_reaction deletes the reaction
     Failure/Error: assert_requested :delete, github_url("/repos/#{@repo.full_name}/releases/#{@release.id}/reactions/#{@reaction.id}")

       The request DELETE https://api.github.com/repos/kfcampbell/an-repo/releases/127001558/reactions/179987855 was expected to execute 1 time but it executed 0 times

       The following requests were made:

       POST https://api.github.com/user/repos with body '{"auto_init":true,"name":"an-repo"}' with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token ghp_TOKEN_REDACTED', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 7.2.0'} was made 1 time
       POST https://api.github.com/repos/kfcampbell/an-repo/releases with body '{"tag_name":"v1.0.0"}' with headers {'Accept'=>'application/vnd.github.v4+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token ghp_TOKEN_REDACTED', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 7.2.0'} was made 1 time
       POST https://api.github.com/repos/kfcampbell/an-repo/releases/127001558/reactions with body '{"content":"+1"}' with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token ghp_TOKEN_REDACTED', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 7.2.0'} was made 1 time
       DELETE https://api.github.com/repos/kfcampbell/an-repo/issues/127001558/reactions/179987855 with body '{}' with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token ghp_TOKEN_REDACTED', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 7.2.0'} was made 1 time

       ============================================================
     # ./spec/octokit/client/reactions_spec.rb:173:in `block (5 levels) in <top (required)>'

  2) Octokit::Client::Reactions with repository with release .create_release_reaction creates a reaction
     Failure/Error: assert_requested :post, github_url("/repos/#{@repo.full_name}/releases/#{@release.id}/reactions")

       The request POST https://api.github.com/repos/kfcampbell/an-repo/releases/127001568/reactions was expected to execute 1 time but it executed 2 times

       The following requests were made:

       POST https://api.github.com/user/repos with body '{"auto_init":true,"name":"an-repo"}' with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token ghp_TOKEN_REDACTED', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 7.2.0'} was made 1 time
       POST https://api.github.com/repos/kfcampbell/an-repo/releases with body '{"tag_name":"v1.0.0"}' with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token ghp_TOKEN_REDACTED', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 7.2.0'} was made 1 time
       POST https://api.github.com/repos/kfcampbell/an-repo/releases/127001568/reactions with body '{"content":"+1"}' with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token ghp_TOKEN_REDACTED', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 7.2.0'} was made 2 times

       ============================================================
     # ./spec/octokit/client/reactions_spec.rb:166:in `block (5 levels) in <top (required)>'

Finished in 11.35 seconds (files took 0.24878 seconds to load)
3 examples, 2 failures

Failed examples:

rspec ./spec/octokit/client/reactions_spec.rb:171 # Octokit::Client::Reactions with repository with release .delete_release_reaction deletes the reaction
rspec ./spec/octokit/client/reactions_spec.rb:163 # Octokit::Client::Reactions with repository with release .create_release_reaction creates a reaction

Which appears to be relevant. Do you have thoughts on that?

@wJoenn
Copy link
Contributor Author

wJoenn commented Oct 27, 2023

Yes, thank you so much @kfcampbell
The first error was because I was using the wrong delete method in my DELETE test. delete_issue_reaction instead of delete_release_reaction because I didn't pay enough attention when copying an existing test.

The second error was caused because I was creating another reaction in my before do instead of only inside the DELETE test which is where I needed it. Because of that the POST request was executed 2 times instead of 1.

Both errors are fixed and all tests pass now 🙌

==> Summary (12 workers in 9.3124s)

    [ 1]                                       9 examples, 0 failures         1 suites in 2.4855s      (pid 9391 exit 0 )
    [ 2]                                      20 examples, 0 failures         1 suites in 9.3084s      (pid 9392 exit 0 )
    [ 3]                                      84 examples, 0 failures         8 suites in 9.3080s      (pid 9393 exit 0 )
    [ 4]                                      70 examples, 0 failures         8 suites in 9.3078s      (pid 9394 exit 0 )
    [ 5]                                     148 examples, 0 failures         6 suites in 9.3075s      (pid 9395 exit 0 )
    [ 6]                                      97 examples, 0 failures         5 suites in 9.3071s      (pid 9396 exit 0 )
    [ 7]                                      68 examples, 0 failures         8 suites in 9.3067s      (pid 9397 exit 0 )
    [ 8]                                      96 examples, 0 failures         6 suites in 9.3064s      (pid 9398 exit 0 )
    [ 9]                                      76 examples, 0 failures         9 suites in 9.3059s      (pid 9399 exit 0 )
    [10]                                     111 examples, 0 failures         5 suites in 9.3041s      (pid 9400 exit 0 )
    [11]                                      63 examples, 0 failures         8 suites in 9.3022s      (pid 9401 exit 0 )
    [12]                                      68 examples, 0 failures         8 suites in 9.3015s      (pid 9402 exit 0 )

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through this @wJoenn!

@kfcampbell kfcampbell merged commit 0ffe565 into octokit:main Oct 27, 2023
12 checks passed
@wJoenn wJoenn deleted the add-missing-reactions-endpoints branch October 28, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEAT]: add missing endpoints
2 participants