-
Notifications
You must be signed in to change notification settings - Fork 381
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
feat: Document/test the behavior of passing an empty string to SetEnv on Windows #3030
Conversation
@remyabel Thanks, LGTM, but I'd like to hear from @coryan too. |
Maybe you can tweak the title, because it just adds some comments? |
Codecov Report
@@ Coverage Diff @@
## master #3030 +/- ##
==========================================
+ Coverage 90.88% 90.96% +0.07%
==========================================
Files 300 300
Lines 20326 20290 -36
==========================================
- Hits 18474 18457 -17
+ Misses 1852 1833 -19
Continue to review full report at Codecov.
|
@remyabel @tmatsuo I definitely do not understand something here, because the tests are failing. |
The tests were failing because I forgot to modify the other tests. I think in the second log this error came up again. |
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.
Reviewed 2 of 4 files at r1, 1 of 2 files at r2.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @remyabel)
google/cloud/internal/env_test.cc, line 35 at r3 (raw file):
/// @test Verify we can unset an environment variable with nullptr. TEST(SetEnv, UnsetEnvWithNullptr) { SetEnv("foo", "");
We could simplify this test by using SetEnv("foo", "bar")
, right? We already checked that setting to an empty string does the expected thing in the previous test?
google/cloud/internal/env_test.cc, line 47 at r3 (raw file):
/// @test Verify we can unset an environment variable. TEST(SetEnv, UnsetEnv) { SetEnv("foo", "");
Ditto?
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.
Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved
Fixes #3027
This is a quick PR to implement my suggestion. Note that I'm not 100% behind this idea because it may be surprising that the env var isn't actually empty when usingSetEnv(var, "")
on Windows.This documents the Windows behavior and adds some tests.
cc @tmatsuo
This change is