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

regexp: get rid of the internal/testenv dependency #31350

Closed
sylvinus opened this issue Apr 8, 2019 · 6 comments
Closed

regexp: get rid of the internal/testenv dependency #31350

sylvinus opened this issue Apr 8, 2019 · 6 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@sylvinus
Copy link
Contributor

sylvinus commented Apr 8, 2019

regexp/exec_test.go depends on internal/testenv for the sole purpose of doing this in 2 tests: isRaceBuilder := strings.HasSuffix(testenv.Builder(), "-race")

Would it be possible to refactor both tests, possibly moving some of their testcases to the exec2_test.go file which has // +build !race? I'm not sure if they are strictly equivalent.

This would make testing patches on the regexp module easier.

thanks!

@ianlancetaylor
Copy link
Contributor

Why does it matter? I run tests via go test regexp. What are you doing?

@sylvinus
Copy link
Contributor Author

sylvinus commented Apr 8, 2019

Sorry I might not be doing the right thing.

I'm in my working copy of src/regexp/ with failing tests. I would like to use my unpatched system go binary to run the tests. when I do that I get this:

➜  regexp git:(master) ✗ go test -run=TestRE2Search
# std/regexp
exec_test.go:11:2: use of internal package internal/testenv not allowed
FAIL	std/regexp [setup failed]

I have to do ../../bin/go test -run=TestRE2Search for them to run. Is there another way?

thanks!

@ianlancetaylor
Copy link
Contributor

I just change the regexp package in GOROOT without running go install cmd/go. So I'm using the unpatched go binary as you suggest. But it's true that it's a little more fragile.

But in general I don't think we should start removing internal/testenv from standard library tests. The argument you are using here would apply to any standard library package, so the end result is that internal/testenv can never be used. That doesn't seem ideal.

@sylvinus
Copy link
Contributor Author

sylvinus commented Apr 8, 2019

Yes I see your point and unfortunately I don't know enough about the whole context to suggest a proper fix. I just wanted to report this is making life a little harder for novice stdlib contributors :)

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 9, 2019
@ALTree ALTree added this to the Unplanned milestone Apr 9, 2019
@rsc
Copy link
Contributor

rsc commented Apr 12, 2019

I'm in my working copy of src/regexp/ with failing tests. I would like to use my unpatched system go binary to run the tests.

If you set GOROOT explicitly to your working copy root, then that should work, provided your system go binary is a close enough version to your working copy. The best answer is to use the cmd/go binary from the working copy, of course. The main Go repo is a whole that moves forward together, not an a la carte menu. There are plenty of other cross-dependencies besides internal/testenv, and that's OK.

@rsc rsc closed this as completed Apr 12, 2019
@sylvinus
Copy link
Contributor Author

sylvinus commented Apr 12, 2019

OK thanks that makes sense.

@golang golang locked and limited conversation to collaborators Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants