github-education-resources / teachers_pet

Command line tool to help teachers use GitHub in their classrooms
https://education.github.com/guide
MIT License
187 stars 74 forks source link

`open_issue` action #38

Closed kelleydv closed 10 years ago

kelleydv commented 10 years ago

Basic rundown:

Multiple issues will require running this multiple times, with multiple issue files, ...

I have verified that this works, but will need some guidance with writing tests. I'm also soliciting feedback regarding idiomatic Ruby. Is there a nicer way to do this?

To Do:

kelleydv commented 10 years ago

User is prompted to optionally add labels separated by commas. No labels, multiple labels, and currently non-existent labels all work as expected.

mikehelmick commented 10 years ago

In general this LGTM, but needs tests.

For help on tests, see this directory https://github.com/education/teachers_pet/tree/master/spec/actions

The specs mimic using the script interactively

kelleydv commented 10 years ago

Thank you! I'm making progress on a test now. I have just a couple questions:

kelleydv commented 10 years ago

Ok, I read the documentation for that part of the github api, and it looks like that post request does indeed create a repo. Sorry for the noob questions, but I'm confused by that.

Edit: I've traced my problem to a misunderstanding of how rspec works... studying now.

kelleydv commented 10 years ago

Wow, I learned a lot - never stubbed requests before, but it's pretty cool.

My test is failing in two of three cases. The two cases are the ones including labels (probably only needs to be one case)

Failures:

  1) TeachersPet::Actions::OpenIssue open issue default labels
     Failure/Error: action.run
     WebMock::NetConnectNotAllowedError:
       Real HTTP connections are disabled. Unregistered request: POST https://testteacher:abc123@api.github.com/repos/testorg/teststudent1-testrepo/issues with body '{"labels":["bug","feature"],"title":"Issue Test","body":"# This is a test\n* Testing\n* An\n* Issue\n"}' with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'User-Agent'=>'Octokit Ruby Gem 3.1.0'}

       You can stub this request with the following snippet:

       stub_request(:post, "https://testteacher:abc123@api.github.com/repos/testorg/teststudent1-testrepo/issues").
         with(:body => "{\"labels\":[\"bug\",\"feature\"],\"title\":\"Issue Test\",\"body\":\"# This is a test\\n* Testing\\n* An\\n* Issue\\n\"}",
              :headers => {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'User-Agent'=>'Octokit Ruby Gem 3.1.0'}).
         to_return(:status => 200, :body => "", :headers => {})

       registered request stubs:

       stub_request(:post, "https://testteacher:abc123@api.github.com/repos/testorg/teststudent3-testrepo/issues").
         with(:body => "{\"labels\":[\"bug\", \"feature\"],\"title\":\"Issue Test\",\"body\":\"# This is a test\\n* Testing\\n* An\\n* Issue\\n\"}")
       stub_request(:post, "https://testteacher:abc123@api.github.com/repos/testorg/teststudent2-testrepo/issues").
         with(:body => "{\"labels\":[\"bug\", \"feature\"],\"title\":\"Issue Test\",\"body\":\"# This is a test\\n* Testing\\n* An\\n* Issue\\n\"}")
       stub_request(:post, "https://testteacher:abc123@api.github.com/repos/testorg/teststudent1-testrepo/issues").
         with(:body => "{\"labels\":[\"bug\", \"feature\"],\"title\":\"Issue Test\",\"body\":\"# This is a test\\n* Testing\\n* An\\n* Issue\\n\"}")
       stub_request(:get, "https://testteacher:abc123@api.github.com/repos/testorg/teststudent3-testrepo")
       stub_request(:get, "https://testteacher:abc123@api.github.com/repos/testorg/teststudent2-testrepo")
       stub_request(:get, "https://testteacher:abc123@api.github.com/repos/testorg/teststudent1-testrepo")
       stub_request(:get, "https://testteacher:abc123@api.github.com/orgs/testorg/teams?per_page=100")
       stub_request(:get, "https://testteacher:abc123@api.github.com/orgs/testorg")

       ============================================================
     # ./lib/teachers_pet/actions/open_issue.rb:61:in `block in create'
     # ./lib/teachers_pet/actions/open_issue.rb:48:in `each'
     # ./lib/teachers_pet/actions/open_issue.rb:48:in `create'
     # ./lib/teachers_pet/actions/open_issue.rb:68:in `run'
     # ./spec/actions/open_issue_spec.rb:57:in `common_test'
     # ./spec/actions/open_issue_spec.rb:69:in `block (2 levels) in <top (required)>'

I am 99% sure it is because the action is making a request including a substring:

"[\"bug\",\"feature\"]"

While my stub provides the substring

"[\"bug\", \"feature\"]"

I.e., there is one extra space in the stub substring. My code for building the stub uses this to make a list of the strings, which I copied directly from octokit.rb.

I was banging my head against the wall for a little while on this. Any suggestions?

Since my test is passing in the "no labels" case, I'm pretty sure this is the problem.

kelleydv commented 10 years ago

Got it with a .delete(' ').

kelleydv commented 10 years ago

Bump! Does this need any polishing?

afeld commented 10 years ago

Sorry for the slow response! Awesome work. Just a few small notes given inline, but otherwise good to go.

Down the road, I think it might make sense to move towards using a "template" repository with the repository contents and issues being copied for each student, so that it more closely mirrors what the students will see.

The comment here suggests the post request that follows is creating repos.

Hmm, that comment isn't really accurate... it's just setting up stubs (read: the expectations) before the action is run. Stubs need to be set up before they are actually called.

I am not sure what this is for.

For better or worse, stubs just block the request from happening, but then they are checked at the end of the script to actually make sure they were called. More info at https://github.com/bblimke/webmock.

kelleydv commented 10 years ago

Thanks!

Stubs need to be set up before they are actually called.

That took me a minute to figure out when I starting writing these tests. At first I thought they were "real" post requests, but it makes a ton of sense now. Glad to have learned this.

I think it might make sense to move towards using a "template" repository

I see what you mean. I think a make repos/push files action should be combined so that they don't have to be separate steps. I have some repos that I want to keep public in my classroom org so that other teachers could use/improve them, and I want to be able to make private copies for my students. Having issues as separate files in that case makes a lot of sense.