Add unit tests for credentials files.#125
Conversation
|
|
||
| TEST_F(EnvironmentTest, ReadApplicationDefaultCredentialsSucceeds) { | ||
| TemporaryFile credentials_file( | ||
| std::string(test_info_->name()) + "_creds.json", |
There was a problem hiding this comment.
where does test_info_ come from?
There was a problem hiding this comment.
test_info_ is provided by gtest.
There was a problem hiding this comment.
Umm, yeah, except the preferred way to access it seems to be by calling current_test_info() on UnitTest. I'll send a separate PR with a fix.
| TEST_F(EnvironmentTest, ReadApplicationDefaultCredentialsCaches) { | ||
| TemporaryFile credentials_file( | ||
| std::string(test_info_->name()) + "_creds.json", | ||
| "{\"client_email\":\"foo@bar.com\",\"private_key\":\"12345\"}"); |
There was a problem hiding this comment.
maybe give these names like:
email: original@bar.com
key: orignal-key
so it is easy to see that the original values were kept in the environment
There was a problem hiding this comment.
I like the approach, but these are too long and don't fit on a line. But see if you like the new code better...
| CPPFLAGS+= -UNOOPT -DNOOPT='[[clang::optnone]]' -I/usr/local/include | ||
| CXXFLAGS+= -Wno-deprecated-declarations -Wno-c++14-extensions | ||
| LDFLAGS+= -L/usr/local/lib | ||
| SED_I+= '' |
There was a problem hiding this comment.
I might be confused here, but it doesn't look like SED_I or SED_EXTRA are used below in this file; do we need these lines?
There was a problem hiding this comment.
They are not, but this is a verbatim copy from src/Makefile — I think it's good to keep them in sync so that we can factor this out.
There was a problem hiding this comment.
Turns out you were right — these weren't needed. Removed.
|
|
||
| TEST_F(EnvironmentTest, ReadApplicationDefaultCredentialsSucceeds) { | ||
| TemporaryFile credentials_file( | ||
| std::string(test_info_->name()) + "_creds.json", |
There was a problem hiding this comment.
test_info_ is provided by gtest.
| "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n" | ||
| )); | ||
| Environment environment(config); | ||
| EXPECT_NO_THROW(ReadApplicationDefaultCredentials(environment)); |
There was a problem hiding this comment.
In my opinion this line is not required. It will be called on any call to CredentialsClientEmail or CredentialsPrivateKey. If it throws, the test case will exit and fail.
There was a problem hiding this comment.
I know, but I'd like to control where the credentials file is actually read. Especially in the next test.
| "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n" | ||
| )); | ||
| Environment environment(config); | ||
| EXPECT_NO_THROW(ReadApplicationDefaultCredentials(environment)); |
There was a problem hiding this comment.
I would personally change this line to something like EXPECT_EQ("foo@bar.com", environment.CredentialsClientEmail());. This will 1) load the cache and 2) demonstrate the expectation (that the cache is loaded) to the test-reader.
There was a problem hiding this comment.
See above. This is checking that the first call to ReadApplicationDefaultCredentials caches both the client email and the private key values.
| CPPFLAGS+= -UNOOPT -DNOOPT='[[clang::optnone]]' -I/usr/local/include | ||
| CXXFLAGS+= -Wno-deprecated-declarations -Wno-c++14-extensions | ||
| LDFLAGS+= -L/usr/local/lib | ||
| SED_I+= '' |
There was a problem hiding this comment.
They are not, but this is a verbatim copy from src/Makefile — I think it's good to keep them in sync so that we can factor this out.
| "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n" | ||
| )); | ||
| Environment environment(config); | ||
| EXPECT_NO_THROW(ReadApplicationDefaultCredentials(environment)); |
There was a problem hiding this comment.
See above. This is checking that the first call to ReadApplicationDefaultCredentials caches both the client email and the private key values.
igorpeshansky
left a comment
igorpeshansky
left a comment
There was a problem hiding this comment.
Also removed the NOOPT macros — I thought they were required on MacOS, but turns out they weren't needed either. PTAL.
| CPPFLAGS+= -UNOOPT -DNOOPT='[[clang::optnone]]' -I/usr/local/include | ||
| CXXFLAGS+= -Wno-deprecated-declarations -Wno-c++14-extensions | ||
| LDFLAGS+= -L/usr/local/lib | ||
| SED_I+= '' |
There was a problem hiding this comment.
Turns out you were right — these weren't needed. Removed.
3 participants
Includes one drive-by include fix in health_check_unittest.