Move GitHub integration tests behind a build flag and add unit tests #527
Conversation
mcastorina
force-pushed
the
github-unit-tests
branch
from
eb4278e
to
8b143ab
Compare
mcastorina
changed the title
| errs := make(chan error, 1) | ||
| reportErr := func(err error) { | ||
| // save the error if there's room, otherwise log and drop it | ||
| select { | ||
| case errs <- err: | ||
| default: | ||
| log.WithError(err).Warn("dropping error") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Doesn't this only ever keep the first error, which I assume would be non fatal if this is called more than once?
There was a problem hiding this comment.
Yes, I believe that was the previous behavior as well? Or rather, the previous behavior saved all errors in a slice and only returned the first if there was one.
There was a problem hiding this comment.
Hmm the previous behavior may not have been correct then. What do you think?
There was a problem hiding this comment.
Hm.. the only advantage I see of returning all the errors is if we want to determine control flow from the specific errors or display them to the user. In its current state we log each error, so we still have the information in the logs.
This function is essentially returning whether 1 or more errors occurred, which I think is fine for the Chunks method.
Additionally, fix some bugs surfaced by unit testing and refactor some of the implementation.