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

testing/iotest: add ErrReader #38781

Closed
caarlos0 opened this issue May 1, 2020 · 14 comments
Closed

testing/iotest: add ErrReader #38781

caarlos0 opened this issue May 1, 2020 · 14 comments

Comments

@caarlos0
Copy link
Contributor

@caarlos0 caarlos0 commented May 1, 2020

Abstract

Add a iotest.ErrReader io.Reader implementation that reads 0 bytes and fails.

This would be useful for quickly adding test cases for when Read doesn't work for some reason.

Background

The iotest package already have a couple of other io.Reader implementations, but we don't have any that fails right away.

On nearly all codebases I worked in, I created this same failReader/errReader implementation. Judging by this and this github searches, I'm not alone.

Proposal

Add it to the standard library, under the iotest package.

I actually have the PR opened already: #34741

Rationale

Make it easier to add test cases for read failures.

@gopherbot gopherbot added this to the Proposal milestone May 1, 2020
@gopherbot gopherbot added the Proposal label May 1, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: add iotest.FailReader proposal: testing/iotest: add FailReader May 1, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals May 1, 2020
@rsc
Copy link
Contributor

@rsc rsc commented May 20, 2020

I'd call it ErrReader, to match DataErrReader, but this seems fine to me.
That is:

package iotest

// ErrReader returns an io.Reader that returns 0, err from all Read calls.
func ErrReader(err error) io.Reader
@caarlos0
Copy link
Contributor Author

@caarlos0 caarlos0 commented May 20, 2020

ErrReader sounds good to me as well. Updated the proposal.

EDIT: updated PR as well.

@caarlos0 caarlos0 changed the title proposal: testing/iotest: add FailReader proposal: testing/iotest: add ErrReader May 20, 2020
@rsc rsc moved this from Incoming to Active in Proposals May 27, 2020
@rsc
Copy link
Contributor

@rsc rsc commented May 27, 2020

Adding to proposal minutes. Seems fine to me, as I said before.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 3, 2020

Based on how trivial this is and the lack of any objections, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Jun 3, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 10, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jun 10, 2020
@rsc rsc modified the milestones: Proposal, Backlog Jun 10, 2020
@rsc rsc changed the title proposal: testing/iotest: add ErrReader testing/iotest: add ErrReader Jun 10, 2020
@networkimprov
Copy link

@networkimprov networkimprov commented Jun 10, 2020

I think the "help wanted" tag got mangled?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 10, 2020

@networkimprov Thanks, fixed.

@caarlos0
Copy link
Contributor Author

@caarlos0 caarlos0 commented Jun 12, 2020

Do I need to do anything else here to get #34741 merged?

@psampaz
Copy link
Contributor

@psampaz psampaz commented Jun 12, 2020

This is a very useful addition in the stdlib.

I was wondering why this has to fail immediately and return 0 bytes read?

If the number of bytes read is set during the tests, ErrReader could potentially cover more cases
including the case of immediate failure using ErrReader(0)

Example:

func ErrReader(n int) io.Reader {
	return &errReader{n}
}

type errReader struct{
       n int
}

func (r *errReader) Read(p []byte) (int, error) {
	return n, ErrIO
}

@caarlos0 does it make sense?

@caarlos0
Copy link
Contributor Author

@caarlos0 caarlos0 commented Jun 12, 2020

@caarlos0 does it make sense?

sounds good to me, not sure if it can be changed now, since it was approved as-is... cc/ @rsc

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 12, 2020

@caarlos0 You don't need to do anything at the moment. We are currently in a release freeze for 1.15. After the 1.15 release goes out, we can review the CL. Thanks for sending it.

Personally I think we should stick with the simple ErrReader. There are an infinite number of possible permutations; we don't want to support all of them.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 16, 2020

Change https://golang.org/cl/199501 mentions this issue: testing/iotest: adds ErrReader

@gopherbot gopherbot closed this in abfeec5 Aug 17, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 17, 2020

Change https://golang.org/cl/248898 mentions this issue: testing/iotest: correct ErrReader signature and remove exported error

gopherbot pushed a commit that referenced this issue Aug 18, 2020
Corrects ErrReader's signature to what was accepted in the approved
proposal, and also removes an exported ErrIO which wasn't part of
the proposal and is unnecessary.

The new signature allows users to customize their own errors.

While here, started examples, with ErrReader leading the way.

Updates #38781

Change-Id: Ia7f84721f11061343cfef8b1adc2b7b69bc3f43c
Reviewed-on: https://go-review.googlesource.com/c/go/+/248898
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 18, 2020

Change https://golang.org/cl/249037 mentions this issue: net/http: use iotest.ErrReader on tests

gopherbot pushed a commit that referenced this issue Aug 21, 2020
Updates #38781

Change-Id: I16a66904167ca4c0e916619b4da1dd23795b3ab2
GitHub-Last-Rev: 4505423
GitHub-Pull-Request: #40864
Reviewed-on: https://go-review.googlesource.com/c/go/+/249037
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.