andygrunwald / go-gerrit

Go client/library for Gerrit Code Review
https://godoc.org/github.com/andygrunwald/go-gerrit
MIT License
96 stars 40 forks source link

Add missing AbandonChange, RebaseChange, RestoreChange and RevertChange #41

Closed opalmer closed 7 years ago

opalmer commented 7 years ago

Discovered AbandonChange was missing when I was working on Change.Abandon() in gerrittest. Figured I'd implement it in go-gerrit and the other missing functions too since they're nearly identical.

codecov-io commented 7 years ago

Codecov Report

Merging #41 into master will increase coverage by 0.97%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   17.94%   18.92%   +0.97%     
==========================================
  Files          21       21              
  Lines        1750     1760      +10     
==========================================
+ Hits          314      333      +19     
+ Misses       1403     1393      -10     
- Partials       33       34       +1
Impacted Files Coverage Δ
changes.go 16.14% <100%> (+11.51%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a2e6113...6c23053. Read the comment docs.

opalmer commented 7 years ago

@andygrunwald, done!

Most of changes.go is missing test coverage including the existing SubmitChange function which is why I didn't add tests initially. But, the new functions are nearly identical so they're simple to test (which is also why I consolidated the code in 75d549b so we're not repeating ourselves unnecessarily.

It would be better if we had a real instance of Gerrit to test against but until gerrittest is finished using the httptest works well for these kinds of changes.

andygrunwald commented 7 years ago

@opalmer Thanks for your effort.

Most of changes.go is missing test coverage

True, i think because i was lazy. Sorry :( But as you wrote, for new functionality it make sense. I am not striving / running for a 100% coverage, but a few tests for the start might be a good idea to motivate more people to go for tests.

It would be better if we had a real instance of Gerrit to test against

Agree. And you are working somehow on it. So tests which is covered twice, doesn't hurt :)

Thank you! I need to serve you a 🍺 or ☕️ or 🍸 some day

opalmer commented 7 years ago

True, i think because i was lazy. Sorry :(

No worries and I'm not trying to blame anyone :). Most parts of go-gerrit are being tested or used in other ways so the lack of coverage is not necessarily an indication that there's a real problem. I brought it up mainly because it's something that could be worked on is all.

Agree. And you are working somehow on it. So tests which is covered twice, doesn't hurt :)

Agreed. gerrittest should solve the 'how do we run and test against a real instance of gerrit'. Matter of fact, that part is already done:

The bits I'm finishing up in gerrittest right now are for another project that I'm hoping to open source soon-ish. It's basically a bot for Gerrit written in Go and since it's using go-gerrit under the hood I wanted to write gerrittest so I could do integration-like testing while at the same time solving #11.

Thank you! I need to serve you a :beer: or :coffee: or :cocktail: some day

Well if you're ever near the Atlanta area let me know, I owe you a drink too for writing go-gerrit in the first place :smile:

andygrunwald commented 7 years ago

Sounds good! What is this gerritbot doing?

I had / started two projects in the past related to bots and gerrit. See https://github.com/andygrunwald/gotrap and https://github.com/andygrunwald/watson. Not my best work, but you know, we learn with every line of code ;)

opalmer commented 7 years ago

Right now what it can do is:

In terms of how it works today:

It's already fairly close to a framework of sorts for interacting with Gerrit but I'm planning to modify the existing bot a bit before making it open source. Functionally it will be the same but I'm going to use go-plugin to connect parts of the bot together so it functions more like this:

Gerrit > Events [events-log plugin or gerrit stream-events] > input plugin > gerritbot [core] > handler plugin > gerritbot > output plugins [email, post comment, etc]

I think output plugins will be fairly limited to common operations because a handler plugin could always do that work itself. The input plugins could do some caching/persistence but I'm trying to keep the core of the bot and the handler plugins event based if that makes any sense.

andygrunwald commented 7 years ago

Sounds great! When this bot-(framework) is done, you can add this into the chapter of go-gerrit as an "Who implemented this lib?" thingy.