[go] testing: add TB.Setenv function
Gerrit Bot (Gerrit)
Gerrit Bot would like Bynovhack to review this change.
testing: add TB.Setenv function
Added Setenv function to TB.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 60a687c2a4c30ca875a60664b19ffcd49d35630f
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 68 insertions(+), 0 deletions(-)
diff --git a/src/testing/testing.go b/src/testing/testing.go
index a44c0a0..33899f8 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -947,6 +947,30 @@
return dir
}
+func (c *common) Setenv(key, value string) {
+ var cleanupFunc = func() {
+ if err := os.Unsetenv(key); err != nil {
+ c.Fatalf("testing: t.Setenv unable to unset env, got %v", err)
+ }
+ }
+
+ // If we already have a value by given key - store it and set it back on cleanup.
+ prevValue, ok := os.LookupEnv(key)
+ if ok {
+ cleanupFunc = func() {
+ if err := os.Setenv(key, prevValue); err != nil {
+ c.Fatalf("testing: t.Setenv unable to set env, got %v", err)
+ }
+ }
+ }
+
+ if err := os.Setenv(key, value); err != nil {
+ c.Fatalf("testing: t.Setenv unable to set env, got %v", err)
+ }
+
+ c.Cleanup(cleanupFunc)
+}
+
// panicHanding is an argument to runCleanup.
type panicHandling int
@@ -1047,6 +1071,14 @@
t.raceErrors += -race.Errors()
}
+func (t *T) Setenv(key, value string) {
+ if t.isParallel {
+ panic("testing: t.Setenv called in parallel mode")
+ }
+
+ t.common.Setenv(key, value)
+}
+
// InternalTest is an internal type but exported because it is cross-package;
// it is part of the implementation of the "go test" command.
type InternalTest struct {
diff --git a/src/testing/testing_test.go b/src/testing/testing_test.go
index d665a33..1ba4d0a 100644
--- a/src/testing/testing_test.go
+++ b/src/testing/testing_test.go
@@ -110,3 +110,39 @@
t.Errorf("unexpected %d files in TempDir: %v", len(fis), fis)
}
}
+
+func TestSetenv(t *testing.T) {
+ // Test case 1
+ if err := os.Setenv("testing_key_1", "previous_value"); err != nil {
+ t.Fatalf("unable to call setenv, got %v", err)
+ }
+
+ t.Run("test_1", func(t *testing.T) {
+ t.Setenv("testing_key_1", "new_value")
+
+ v := os.Getenv("testing_key_1")
+ if v != "new_value" {
+ t.Fatalf("test_1: expected: %s, got: %s", "new_value", v)
+ }
+ })
+
+ v := os.Getenv("testing_key_1")
+ if v != "previous_value" {
+ t.Fatalf("test_1: expected: %s, got: %s", "previous_value", v)
+ }
+
+ // Test case 2
+ t.Run("test_2", func(t *testing.T) {
+ t.Setenv("testing_key_2", "new_value")
+
+ v := os.Getenv("testing_key_2")
+ if v != "new_value" {
+ t.Fatalf("test_2: expected: %s, got: %s", "new_value", v)
+ }
+ })
+
+ v = os.Getenv("testing_key_2")
+ if v != "" {
+ t.Fatalf("test_2: expected: %s, got: %s", "", v)
+ }
+}
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
1 comment:
Patchset:
We should also give an error if t.Parallel is used after t.Setenv.
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Patch set 1:Run-TryBot +1
5 comments:
Commit Message:
testing: add TB.Setenv function
Added Setenv function to TB.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Patchset:
Thank you for this change Alexey! Thank you Ian for the review too.
Alexey, I've provided some review, but also I am kindly tagging some
more interested parties.
File src/testing/testing.go:
Patch Set #1, Line 953:
testing: t.Setenvc.Fatalf("Setenv: unable to unset %q, got %v", key, err)
var cleanupFunc = func() {
if err := os.Unsetenv(key); err != nil {
c.Fatalf("testing: t.Setenv unable to unset env, got %v", err)
}
}
// If we already have a value by given key - store it and set it back on cleanup.
prevValue, ok := os.LookupEnv(key)
if ok {
cleanupFunc = func() {
if err := os.Setenv(key, prevValue); err != nil {
c.Fatalf("testing: t.Setenv unable to set env, got %v", err)
}
}
}
We can group these by:
a) Firstly checking if an original value existed
b) If ok, then use os.Setenv otherwise, unsetenv soprevious, ok := os.LookupEnv(key)
cleanupFunc := func() {
switch ok {
case true:
if err := os.Setenv(key, previous); err != nil {
c.Fatalf("Setenv: unable to revert %q to previous value: %v", key, err)
}
default:
if err := os.Unsetenv(key); err != nil {
c.Fatalf("Setenv: unable to unset %q: %v", key, err)
}
}
}c.Cleanup(cleanupFunc)
File src/testing/testing_test.go:
func TestSetenv(t *testing.T) {
// Test case 1
if err := os.Setenv("testing_key_1", "previous_value"); err != nil {
t.Fatalf("unable to call setenv, got %v", err)
}
t.Run("test_1", func(t *testing.T) {
t.Setenv("testing_key_1", "new_value")
v := os.Getenv("testing_key_1")
if v != "new_value" {
t.Fatalf("test_1: expected: %s, got: %s", "new_value", v)
}
})
v := os.Getenv("testing_key_1")
if v != "previous_value" {
t.Fatalf("test_1: expected: %s, got: %s", "previous_value", v)
}
// Test case 2
t.Run("test_2", func(t *testing.T) {
t.Setenv("testing_key_2", "new_value")
v := os.Getenv("testing_key_2")
if v != "new_value" {
t.Fatalf("test_2: expected: %s, got: %s", "new_value", v)
}
})
v = os.Getenv("testing_key_2")
if v != "" {
t.Fatalf("test_2: expected: %s, got: %s", "", v)
}
}
I believe we should isolate these cases into 2 and then have subtests with more descriptive names so:
const setEnvKey = "key1"
func TestSetenv_withoutPriorVariableSet(t *testing.T) {
testSetenv(t, "")
}func TestSetenv_withPriorVariableSet(t *testing.T) {
initialValue := "initial-value"
os.Setenv(setEnvKey, initialValue)
defer os.Unsetenv(setEnvKey)testSetenv(t, initialValue)
}func testSetenv(t *testing.T, initialValue string) {
key := setEnvKey
if g, w := os.Getenv(key), initialValue; g != w {
t.Fatalf("Mismatch: key1's original value = %q, want %q", g, w)
}// Now set the value.
t.Setenv(key, "value1")
if g, w := os.Getenv(key), "value1"; g != w {
t.Fatalf("Mismatch: key1's value = %q, want %q", g, w)
}// Now change that value in a subtest.
t.Run("SubTest and setEnv", func(t *testing.T) {
if g, w := os.Getenv(key), "value1"; g != w {
t.Fatalf("Mismatch: key1's value = %q, want %q", g, w)
}
t.Setenv(key, "subValue1")
if g, w := os.Getenv(key), "subValue1"; g != w {
t.Fatalf("Mismatch: key1's value = %q, want %q", g, w)
}
})// Ensure that after the subtest has run, that the variable hasn't changed.
if g, w := os.Getenv(key), "value1"; g != w {
t.Fatalf("Mismatch: key1's value = %q, want %q", g, w)
}
}And then we also need a test to ensure that after TB.Parallel() are invoked, that it t.Fatals appropriately, please examine the code in this file or related for how to test common.
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
1 comment:
Patchset:
Thank you for this change Alexey! Thank you Ian for the review too. […]
Thank you for review and good points! I will work on it in several days
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #2 to this change.
testing: add TB.Setenv function
Added Setenv function to TB.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 83ed4a79bcae66d83d051c7c2e0753efe8333011
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 68 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #3 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 83ed4a79bcae66d83d051c7c2e0753efe8333011
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 68 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #4 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: ad6030ab7eb13448010f3810ab3896a1025c1380
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 113 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
4 comments:
Commit Message:
testing: add TB.Setenv function
Added Setenv function to TB.
testing: add TB.Setenv […]
Done
File src/testing/testing.go:
Patch Set #1, Line 953:
testing: t.Setenv
c. […]
Done
var cleanupFunc = func() {
if err := os.Unsetenv(key); err != nil {
c.Fatalf("testing: t.Setenv unable to unset env, got %v", err)
}
}
// If we already have a value by given key - store it and set it back on cleanup.
prevValue, ok := os.LookupEnv(key)
if ok {
cleanupFunc = func() {
if err := os.Setenv(key, prevValue); err != nil {
c.Fatalf("testing: t.Setenv unable to set env, got %v", err)
}
}
}
We can group these by: […]
Done
File src/testing/testing_test.go:
I believe we should isolate these cases into 2 and then have subtests with more descriptive names so […]
Done
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Patch set 4:Run-TryBot +1Code-Review +2Trust +1
1 comment:
Patchset:
Nice, thank you Alexey! Ian, please help with another look and a Trust+1, thank you!
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
1 comment:
File src/testing/testing_test.go:
Patch Set #4, Line 14:
const setEnvKey = "TEST_KEY_1"Please start this with "GO", as in "GO_TEST_KEY_1", to make it less likely to collide with any existing environment variable. Thanks.
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #5 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 00ebd46808de19c6f807383462b95d1268a4bdd6
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 113 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
1 comment:
File src/testing/testing_test.go:
Patch Set #4, Line 14:
const setEnvKey = "TEST_KEY_1"
Please start this with "GO", as in "GO_TEST_KEY_1", to make it less likely to collide with any exist […]
Done
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
roger peppe (Gerrit)
Patch set 5:Code-Review -1
9 comments:
Patchset:
Looks pretty good in general, with a few comments (after writing the comments, I realised that some of them go against suggestions from Emmanuel (sorry Emmanuel!) - feel free to go in either direction :) ). We should definitely test the Unsetenv case, and I do feel that independent tests would make things a little easier to see which cases are being tested.
File src/testing/testing.go:
Patch Set #5, Line 955:
switch ok {Using switch seems odd here. why not just "if ok" ?
Also, I don't really see there's a need for cleanupFunc at all.
And I'm not sure that it's worth checking the result of the cleanup operations - the only way that Setenv can fail is if the key or the value is invalid, and we know they're both OK because they came from LookupEnv.How about something like this instead?
prevValue, ok := os.LookupEnv(key)
if err := os.Setenv(key, value); err != nil {
c.Fatalf("cannot set environment variable: %v", err)
}
if ok {
c.Cleanup(func() {
os.Setenv(key, prevValue)
})
} else {
c.Cleanup(func() {
os.Unsetenv(key)
}
}Patch Set #5, Line 1043:
panic("testing: t.Parallel called after setenv")perhaps a slightly more self-explanatory message might be useful:
panic("testing: t.Parallel called after t.Setenv; cannot set environment variables in parallel tests")Patch Set #5, Line 1080:
panic("testing: t.Setenv called in parallel mode")perhaps a slightly more self-explanatory message might be useful:
panic("testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests")
File src/testing/testing_test.go:
Patch Set #5, Line 127:
func TestSetenvWithParallelAfterSetnev(t *testing.T) {s/Setnev/Setenv/
Patch Set #5, Line 130:
t.Errorf("Setenv: panic expected")Perhaps check the panic string to make sure it's the one we're expecting?
Patch Set #5, Line 142:
func TestSetenvWithParallelBeforeSetnev(t *testing.T) {s/Setnev/Setenv/
Patch Set #5, Line 145:
t.Errorf("Setenv: panic expected")Perhaps check the panic string to make sure it's the one we're expecting?
Patch Set #5, Line 159:
if g, w := os.Getenv(key), initialValue; g != w {Given that we're dealing with potentially unset environment variables, wouldn't it be better to pass in whether the variable is expected to be set too, and use LookupEnv to check the initial state.
Also, this shared test code doesn't actually seem to be testing the behaviour when there's no initial environment variable set. I'm not convinced that it's actually worth having a shared test function here tbh, over just having specific code in each test.
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Patch set 5:-Code-Review
1 comment:
Patchset:
Thank you Roger for chiming in, no biggie I’ll yield to you for the review and I tagged you purposefully to chime in. Thank you Alexey for the work on this, and please go on with Roger’s suggestions. Roger, and others, please don’t hesitate to ping me if you need anything.
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #6 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: a2c151b0f06f205692e861029343ad7fd96127fc
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 109 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
8 comments:
File src/testing/testing.go:
Patch Set #5, Line 955:
switch ok {
Using switch seems odd here. why not just "if ok" ? […]
Done
Patch Set #5, Line 1043:
panic("testing: t.Parallel called after setenv")
perhaps a slightly more self-explanatory message might be useful: […]
Done
Patch Set #5, Line 1080:
panic("testing: t.Setenv called in parallel mode")
perhaps a slightly more self-explanatory message might be useful: […]
Done
File src/testing/testing_test.go:
Patch Set #5, Line 127:
func TestSetenvWithParallelAfterSetnev(t *testing.T) {s/Setnev/Setenv/
Done
Patch Set #5, Line 130:
t.Errorf("Setenv: panic expected")Perhaps check the panic string to make sure it's the one we're expecting?
Done
Patch Set #5, Line 142:
func TestSetenvWithParallelBeforeSetnev(t *testing.T) {s/Setnev/Setenv/
Done
Patch Set #5, Line 145:
t.Errorf("Setenv: panic expected")Perhaps check the panic string to make sure it's the one we're expecting?
Done
Patch Set #5, Line 159:
if g, w := os.Getenv(key), initialValue; g != w {
Given that we're dealing with potentially unset environment variables, wouldn't it be better to pass […]
Done
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Changkun Ou (Gerrit)
2 comments:
File src/testing/testing.go:
Patch Set #6, Line 1073:
func (t *T) Setenv(key, value string) {The new API should be documented, something maybe:
```
// Setenv sets an environment variable for the lifetime of a test, and
// it will be unset automatically when the test exits.
// Tests that invokes Setenv cannot be executed in parallel.
func (t *T) Setenv(key, value string) {
```
Patch Set #6, Line 1073:
func (t *T) Setenv(key, value string) {I don't quite understand how this helper should really behave.
For instance:```
func TestSetenvMisuse(t *testing.T) {
t.Run("x", func(t *testing.T) {
t.Setenv("a", "b")
t.Run("y", func(t *testing.T) {
os.Setenv("a", "c")
})
if os.Getenv("a") != "b" {
t.Fatalf("want b got %v", os.Getenv("a"))
}
})
}
```What if a subtest changes the environment variable using os.Setenv instead of t.Setenv? The current implementation does not guarantee the immutability of an environment variable set by t.Setenv, this test will fail.
If the above test is considered as a misuse, is it inconsistent with the document which says "the lifetime of a test"?
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
roger peppe (Gerrit)
Patch set 6:Code-Review -1
9 comments:
Patchset:
Thanks very much for your changes! We're going in the right direction, but still a few things to sort out.
File src/testing/testing.go:
Patch Set #6, Line 951:
func (c *common) Setenv(key, value string) {Please duplicate the doc comment here.
Patch Set #6, Line 958:
// If we already have value by a given key - store it and set it back on cleanup.I'm not sure this comment adds much value.
Patch Set #6, Line 1073:
func (t *T) Setenv(key, value string) {
I don't quite understand how this helper should really behave. […]
It's not possible to mark an environment variable as immutable, and it's probably not desirable either (it's quite possible that we'd like to test code that mutates environment variables), so I think it's fine that the above code will fail.
Patch Set #6, Line 1073:
func (t *T) Setenv(key, value string) {
The new API should be documented, something maybe: […]
Yes, we need a doc comment! I'd suggest something which promises a bit less though:
Something like this, perhaps?
```
// Setenv calls os.Setenv(key, value) and uses Cleanup to
// restore the environment variable to its original value
// after the test.
//
// This cannot be used in parallel tests.
```
File src/testing/testing_test.go:
Patch Set #6, Line 114:
func TestSetenv(t *testing.T) {This test should also test the behaviour when the value was initially unset, please.
Patch Set #6, Line 140:
if os.Getenv(test.key) != test.initialValue {Why are we testing that `os.Setenv` is setting an environment variable here? I think it's OK to rely on that tbh.
Patch Set #6, Line 161:
if r := recover(); r != nil {We want to test the value of r regardless of whether it's nil - it's a bug if this code doesn't panic, but the test as is will pass in that case.
Patch Set #6, Line 177:
if r := recover(); r != nil {similar to above, the test should fail if the code doesn't panic.
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
8 comments:
File src/testing/testing.go:
Patch Set #6, Line 951:
func (c *common) Setenv(key, value string) {Please duplicate the doc comment here.
Done
Patch Set #6, Line 958:
// If we already have value by a given key - store it and set it back on cleanup.I'm not sure this comment adds much value.
Done
Patch Set #6, Line 1073:
func (t *T) Setenv(key, value string) {
It's not possible to mark an environment variable as immutable, and it's probably not desirable eith […]
I've left this part of code without any edits.
Patch Set #6, Line 1073:
func (t *T) Setenv(key, value string) {
Yes, we need a doc comment! I'd suggest something which promises a bit less though: […]
Done
File src/testing/testing_test.go:
Patch Set #6, Line 114:
func TestSetenv(t *testing.T) {This test should also test the behaviour when the value was initially unset, please.
But I've already tested for it - in second test case we do not have an initial value.
Correct me if I wrong.
Patch Set #6, Line 140:
if os.Getenv(test.key) != test.initialValue {
Why are we testing that `os. […]
Done
Patch Set #6, Line 161:
if r := recover(); r != nil {
We want to test the value of r regardless of whether it's nil - it's a bug if this code doesn't pani […]
Done
Patch Set #6, Line 177:
if r := recover(); r != nil {similar to above, the test should fail if the code doesn't panic.
Done
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
roger peppe (Gerrit)
3 comments:
Patchset:
Nearly there, thanks! We still need a test for the unset case, and one other minor suggestion for the panic check in the tests.
File src/testing/testing_test.go:
Patch Set #6, Line 114:
func TestSetenv(t *testing.T) {
But I've already tested for it - in second test case we do not have an initial value. […]
An empty value is not the same as an unset value. There is specific logic in the code to deal with this, which is why it needs a test case, please.
Patch Set #6, Line 161:
if r := recover(); r != nil {
Done
Or just:
want := "testing: t.Parallel called after t.Setenv; cannot set environment variables in parallel tests"
if got := recover(); got != want {
t.Fatalf("expected panic; got %#v want %q", got, want)
}(Note: the Go tests usually use "want" instead of "expected" and usually print "got" before "want").
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #8 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 5a5080780b328278932400e5e1f3d98f714ab872
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 109 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
2 comments:
File src/testing/testing_test.go:
Patch Set #6, Line 114:
func TestSetenv(t *testing.T) {
An empty value is not the same as an unset value. […]
I'm not setting an initial value if it equal to "" at testing_test.go:L136
And in second test case we have unset value, not emtpty.
Patch Set #6, Line 161:
if r := recover(); r != nil {
Or just: […]
Done
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
roger peppe (Gerrit)
Attention is currently required from: Bynovhack.
5 comments:
Patchset:
A few more minor nits and then we're good. Thanks, and sorry for the delayed review. We _will_ get this in Go 1.17 😊
File src/testing/testing_test.go:
Patch Set #6, Line 114:
func TestSetenv(t *testing.T) {
I'm not setting an initial value if it equal to "" at testing_test.go:L136 […]
You're not testing that the value is in fact unset after the test - just that `os.Getenv` returns the empty string, which is not quite the same.
I'd say there at least three distinct cases to test here:
- initial value is non-empty
- initial value is set but empty
- initial value is unset
I'd suggest it's worth testing all of them, and also not just assuming that a value is unset just because you haven't set it, but explicitly using `os.Unsetenv` instead.
File src/testing/testing_test.go:
Patch Set #8, Line 115:
var tests = []struct {tests := []struct {
Patch Set #8, Line 157:
var want = "testing: t.Parallel called after t.Setenv; cannot set environment variables in parallel tests"want :=
Patch Set #8, Line 170:
var want = "testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests"want :=
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #9 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 9448937fa1962b9f6b6935e74e8e47fd0bd1dcb6
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 123 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
Attention is currently required from: roger peppe.
4 comments:
File src/testing/testing_test.go:
Patch Set #6, Line 114:
func TestSetenv(t *testing.T) {
You're not testing that the value is in fact unset after the test - just that `os. […]
I've added one more test case with empty initial value and added testing not only for `os.Getenv != test.initialValue` but for existing with `os.LookupEnv`
File src/testing/testing_test.go:
Patch Set #8, Line 115:
var tests = []struct {tests := []struct {
Done
Patch Set #8, Line 157:
var want = "testing: t.Parallel called after t.Setenv; cannot set environment variables in parallel tests"want :=
Done
Patch Set #8, Line 170:
var want = "testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests"want :=
Done
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
roger peppe (Gerrit)
Patch set 9:Code-Review +2
3 comments:
Patchset:
LGTM with a couple more trivial things. Thanks!
File src/testing/testing_test.go:
else {
os.Unsetenv(test.key)
}That makes sure that the test will succeed even if someone randomly happens to have an environment variable named GO_TEST_KEY_3. Also, if you do that, you don't even really need a different key for each test.
Patch Set #9, Line 164:
t.Fatalf("unexpected value after t.Setnev cleanup: got %t, want %t", exists, test.initialValueExists)s/Setnev/Setenv/
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #10 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 712867aae2267393d1d958b12217626608ed8aea
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 125 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
2 comments:
File src/testing/testing_test.go:
else { […]
Done
Patch Set #9, Line 164:
t.Fatalf("unexpected value after t.Setnev cleanup: got %t, want %t", exists, test.initialValueExists)s/Setnev/Setenv/
Done
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Patch set 10:Run-TryBot +1Trust +1
1 comment:
Patchset:
Thank you, Bynohack for the changes, and Roger for the review!
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
Attention is currently required from: Emmanuel Odeke.
1 comment:
Patchset:
Thank you, Bynohack for the changes, and Roger for the review!
Thank you all!
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
Patchset:
The parent is too old; you should rebase your PR on the latest master.
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Emmanuel Odeke.
Gerrit Bot uploaded patch set #11 to this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 0ca12fa565318f350b927e2ef94f3b4f792c75c2
GitHub-Pull-Request: golang/go#41857
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 125 insertions(+), 0 deletions(-)
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Bynovhack (Gerrit)
Attention is currently required from: Daniel Martí, Emmanuel Odeke.
1 comment:
Patchset:
The parent is too old; you should rebase your PR on the latest master.
Rebased
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
Attention is currently required from: Emmanuel Odeke.
Patch set 11:Run-TryBot +1Trust +1
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
Daniel Martí submitted this change.
testing: add TB.Setenv
Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.
Fixes #41260
Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 0ca12fa565318f350b927e2ef94f3b4f792c75c2
GitHub-Pull-Request: golang/go#41857
Reviewed-on: https://go-review.googlesource.com/c/go/+/260577
Trust: Daniel Martí <mv...@mvdan.cc>
Trust: Emmanuel Odeke <emma...@orijtech.com>
Run-TryBot: Daniel Martí <mv...@mvdan.cc>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: roger peppe <rogp...@gmail.com>
---
M src/testing/testing.go
M src/testing/testing_test.go
2 files changed, 125 insertions(+), 0 deletions(-)
diff --git a/src/testing/testing.go b/src/testing/testing.go
index 466dd96..fc52f3c 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -667,6 +667,7 @@
type T struct {
common
isParallel bool
+ isEnvSet bool
context *testContext // For running tests and subtests.
}
@@ -964,6 +965,29 @@
return dir
}
+// Setenv calls os.Setenv(key, value) and uses Cleanup to
+// restore the environment variable to its original value
+// after the test.
+//
+// This cannot be used in parallel tests.
+func (c *common) Setenv(key, value string) {
+ prevValue, ok := os.LookupEnv(key)
+
+ if err := os.Setenv(key, value); err != nil {
+ c.Fatalf("cannot set environment variable: %v", err)
+ }
+
+ if ok {
+ c.Cleanup(func() {
+ os.Setenv(key, prevValue)
+ })
+ } else {
+ c.Cleanup(func() {
+ os.Unsetenv(key)
+ })
+ }
+}
+
// panicHanding is an argument to runCleanup.
type panicHandling int
@@ -1035,6 +1059,9 @@
if t.isParallel {
panic("testing: t.Parallel called multiple times")
}
+ if t.isEnvSet {
+ panic("testing: t.Parallel called after t.Setenv; cannot set environment variables in parallel tests")
+ }
t.isParallel = true
// We don't want to include the time we spend waiting for serial tests
@@ -1068,6 +1095,21 @@
t.raceErrors += -race.Errors()
}
+// Setenv calls os.Setenv(key, value) and uses Cleanup to
+// restore the environment variable to its original value
+// after the test.
+//
+// This cannot be used in parallel tests.
+func (t *T) Setenv(key, value string) {
+ if t.isParallel {
+ panic("testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests")
+ }
+
+ t.isEnvSet = true
+
+ t.common.Setenv(key, value)
+}
+
// InternalTest is an internal type but exported because it is cross-package;
// it is part of the implementation of the "go test" command.
type InternalTest struct {
diff --git a/src/testing/testing_test.go b/src/testing/testing_test.go
index 0f09698..55a4df4 100644
--- a/src/testing/testing_test.go
+++ b/src/testing/testing_test.go
@@ -109,3 +109,86 @@
t.Errorf("unexpected %d files in TempDir: %v", len(files), files)
}
}
+
+func TestSetenv(t *testing.T) {
+ tests := []struct {
+ name string
+ key string
+ initialValueExists bool
+ initialValue string
+ newValue string
+ }{
+ {
+ name: "initial value exists",
+ key: "GO_TEST_KEY_1",
+ initialValueExists: true,
+ initialValue: "111",
+ newValue: "222",
+ },
+ {
+ name: "initial value exists but empty",
+ key: "GO_TEST_KEY_2",
+ initialValueExists: true,
+ initialValue: "",
+ newValue: "222",
+ },
+ {
+ name: "initial value is not exists",
+ key: "GO_TEST_KEY_3",
+ initialValueExists: false,
+ initialValue: "",
+ newValue: "222",
+ },
+ }
+
+ for _, test := range tests {
+ if test.initialValueExists {
+ if err := os.Setenv(test.key, test.initialValue); err != nil {
+ t.Fatalf("unable to set env: got %v", err)
+ }
+ } else {
+ os.Unsetenv(test.key)
+ }
+
+ t.Run(test.name, func(t *testing.T) {
+ t.Setenv(test.key, test.newValue)
+ if os.Getenv(test.key) != test.newValue {
+ t.Fatalf("unexpected value after t.Setenv: got %s, want %s", os.Getenv(test.key), test.newValue)
+ }
+ })
+
+ got, exists := os.LookupEnv(test.key)
+ if got != test.initialValue {
+ t.Fatalf("unexpected value after t.Setenv cleanup: got %s, want %s", got, test.initialValue)
+ }
+ if exists != test.initialValueExists {
+ t.Fatalf("unexpected value after t.Setenv cleanup: got %t, want %t", exists, test.initialValueExists)
+ }
+ }
+}
+
+func TestSetenvWithParallelAfterSetenv(t *testing.T) {
+ defer func() {
+ want := "testing: t.Parallel called after t.Setenv; cannot set environment variables in parallel tests"
+ if got := recover(); got != want {
+ t.Fatalf("expected panic; got %#v want %q", got, want)
+ }
+ }()
+
+ t.Setenv("GO_TEST_KEY_1", "value")
+
+ t.Parallel()
+}
+
+func TestSetenvWithParallelBeforeSetenv(t *testing.T) {
+ defer func() {
+ want := "testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests"
+ if got := recover(); got != want {
+ t.Fatalf("expected panic; got %#v want %q", got, want)
+ }
+ }()
+
+ t.Parallel()
+
+ t.Setenv("GO_TEST_KEY_1", "value")
+}
To view, visit change 260577. To unsubscribe, or for help writing mail filters, visit settings.