Conversation
iainlane
force-pushed
the
iainlane/testability
branch
from
9b29ce5 to
e92072b
Compare
| } | ||
|
|
||
| func handleCIStatus(status github.CIStatus) cli.ExitCoder { | ||
| func handleCIStatus(status github.CIStatus, recheckInterval time.Duration) cli.ExitCoder { |
There was a problem hiding this comment.
Feels weird to have this new param just for logging purposes. This function looks like it shouldn't know about the interval and the log should be at a higher level
There was a problem hiding this comment.
Yeah. We should pass the logger via the struct or as a parameter or something, and have the config we want to log set as a field on that at a higher level. We could probably set that in the root even, to be honest. I'll look at doing that in a follow-up, cheers.
cmd/wait-for-github/ci.go
Outdated
|
|
||
| func checkCIStatus(c *cli.Context) error { | ||
| ctx := context.Background() | ||
| type checkSpecificCI struct { |
There was a problem hiding this comment.
checkSpecificCI. Not sure about the name. What part is specific?
There was a problem hiding this comment.
It's the checks field. When we use this one we call GetCIStatusForChecks, it takes a slice of check/status names to look for and makes sure they are all there.
However you made me realise that the structs could be a bit better if the checks slice wasn't in the checkAllCI one, so I reorganised them and re-pushed. Thanks for that 👍
iainlane
force-pushed
the
iainlane/testability
branch
from
e92072b to
781f8e6
Compare
In order to make the code testable one thing we need to do is eliminate the global state. That's what we do here. The handlers are turned into functions and they can have their own local state which they pass around.
Add a `Check` interface. This lets us separate out the part which is repeatedly called into their own functions. Then they'll be able to be tested.
Boilerplate removal
In an upcoming commit we will be testing the `ci` and `pr` subcommands. Splitting the interfaces into smaller components will make this a bit easier, since those testsuites will only have to fake what they need.
Now we've done all that refactoring, we can start to add more testcases sensibly. Actually, this particular commit would have been more or less possible already. Test the low-level github internal module. We've aimed for as close to 100% coverage as we can get, so there are a lot of cases.
iainlane
force-pushed
the
iainlane/testability
branch
from
781f8e6 to
760880a
Compare
2 participants
This is part of a PR stack:
I started to add a new feature in #60, and wanted to add tests for it. But it turned out to be quite difficult due to the global state we have in the project, and the way the commands are constructed.
So here's a pre-PR with quite a bit of reorganisation of the code. I'll follow up with tests for the CI and PR subcommands, and then the new feature™.
Have tried to split the work into logical commits with each commit having a decent message, but if this is still too big to review then I can push each of those as separate PRs, just let me know. 👍