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 · 15 comments
Closed

testing/iotest: add ErrReader #38781

caarlos0 opened this issue May 1, 2020 · 15 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@caarlos0
Copy link
Contributor

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
@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 (old) May 1, 2020
@rsc
Copy link
Contributor

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 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 (old) May 27, 2020
@rsc
Copy link
Contributor

rsc commented May 27, 2020

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

@rsc
Copy link
Contributor

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 (old) Jun 3, 2020
@rsc
Copy link
Contributor

rsc commented Jun 10, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jun 10, 2020
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels 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

I think the "help wanted" tag got mangled?

@ianlancetaylor
Copy link
Contributor

@networkimprov Thanks, fixed.

@caarlos0
Copy link
Contributor Author

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

@psampaz
Copy link
Contributor

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 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

@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

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

@gopherbot
Copy link

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

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>
@gopherbot
Copy link

Change https://golang.org/cl/285673 mentions this issue: doc/go1.16: mention new testing/iotest functions

gopherbot pushed a commit that referenced this issue Jan 25, 2021
For #38781
For #40700
For #41190

Change-Id: I72f1055e51edb517041d3861640734ba6ef5f342
Reviewed-on: https://go-review.googlesource.com/c/go/+/285673
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Jan 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants