-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
test(storage): add test for uploading with an emulator #4641
test(storage): add test for uploading with an emulator #4641
Conversation
Variable seems to be duplicating information which led to a bug.
…into remove-envhost
You can now use the client library with an emulator by setting the environment variable to a host, with or without a scheme You also no longer have to supply an endpoint to use the emulator; the endpoint is built for you
storage/storage_test.go
Outdated
} | ||
ctx := context.Background() | ||
for _, tc := range testCases { | ||
os.Setenv("STORAGE_EMULATOR_HOST", tc.StorageEmulatorHost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will change the value of the env var on the system (and mess up subsequent integration tests). You should capture the existing value for this in a variable and defer a call to restore the value to what it originally was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's what I'm doing on line 1368 and 1369
…google-cloud-go into add-emulator-ops-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Two small things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice addition!
case <-timeout: | ||
t.Errorf("test timeout") | ||
timedOut <- true | ||
case <-done: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could've used a WaitGroup (this is fine though!)
This test should catch an error such as #2476
Also added tests to ensure endpoint overrides emulator host when host is specified as host:port or scheme://host:port - this adds extra coverage to ensure something like #4444 does not occur again