From fb77ddbc45f2c2bd3392978101cc2856227c783a Mon Sep 17 00:00:00 2001 From: Qian Sun Date: Wed, 11 Jan 2017 14:30:16 -0800 Subject: [PATCH 1/2] Add unit test. --- mixerclient/BUILD | 11 + mixerclient/BUILD.googletest | 29 +++ mixerclient/WORKSPACE | 17 ++ mixerclient/src/client_impl_test.cc | 319 ++++++++++++++++++++++++++++ 4 files changed, 376 insertions(+) create mode 100644 mixerclient/BUILD.googletest create mode 100644 mixerclient/src/client_impl_test.cc diff --git a/mixerclient/BUILD b/mixerclient/BUILD index bd282ebe0dcd..0472f4947de5 100644 --- a/mixerclient/BUILD +++ b/mixerclient/BUILD @@ -31,4 +31,15 @@ cc_library( "//external:boringssl_crypto", "//external:mixer_api_cc_proto", ], +) + +cc_test( + name = "mixer_client_impl_test", + size = "small", + srcs = ["src/client_impl_test.cc"], + linkopts = ["-lm"], + deps = [ + ":mixer_client_lib", + "//external:googletest_main", + ], ) \ No newline at end of file diff --git a/mixerclient/BUILD.googletest b/mixerclient/BUILD.googletest new file mode 100644 index 000000000000..5acacb47cc01 --- /dev/null +++ b/mixerclient/BUILD.googletest @@ -0,0 +1,29 @@ + +cc_library( + name = "googletest", + srcs = [ + "googletest/src/gtest-all.cc", + "googlemock/src/gmock-all.cc", + ], + hdrs = glob([ + "googletest/include/**/*.h", + "googlemock/include/**/*.h", + "googletest/src/*.cc", + "googletest/src/*.h", + "googlemock/src/*.cc", + ]), + includes = [ + "googlemock", + "googletest", + "googletest/include", + "googlemock/include", + ], + visibility = ["//visibility:public"], +) + +cc_library( + name = "googletest_main", + srcs = ["googlemock/src/gmock_main.cc"], + visibility = ["//visibility:public"], + deps = [":googletest"], +) diff --git a/mixerclient/WORKSPACE b/mixerclient/WORKSPACE index 9524fad82aec..d48f97f53629 100644 --- a/mixerclient/WORKSPACE +++ b/mixerclient/WORKSPACE @@ -77,4 +77,21 @@ new_git_repository( bind( name = "mixer_api_cc_proto", actual = "@mixerapi_git//:mixer_api_cc_proto", +) + +new_git_repository( + name = "googletest_git", + commit = "ec44c6c1675c25b9827aacd08c02433cccde7780", + remote = "https://github.com/google/googletest.git", + build_file = "BUILD.googletest", +) + +bind( + name = "googletest", + actual = "@googletest_git//:googletest", +) + +bind( + name = "googletest_main", + actual = "@googletest_git//:googletest_main", ) \ No newline at end of file diff --git a/mixerclient/src/client_impl_test.cc b/mixerclient/src/client_impl_test.cc new file mode 100644 index 000000000000..68c3a3d2cdf4 --- /dev/null +++ b/mixerclient/src/client_impl_test.cc @@ -0,0 +1,319 @@ +/* Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "include/client.h" + +#include +#include "gmock/gmock.h" +#include "google/protobuf/text_format.h" +#include "google/protobuf/util/message_differencer.h" +#include "gtest/gtest.h" +using ::istio::mixer::v1::CheckRequest; +using ::istio::mixer::v1::CheckResponse; +using ::istio::mixer::v1::ReportRequest; +using ::istio::mixer::v1::ReportResponse; +using ::istio::mixer::v1::QuotaRequest; +using ::istio::mixer::v1::QuotaResponse; +using ::google::protobuf::TextFormat; +using ::google::protobuf::util::MessageDifferencer; +using ::google::protobuf::util::Status; +using ::google::protobuf::util::error::Code; +using ::testing::Invoke; +using ::testing::Mock; +using ::testing::_; + +namespace istio { +namespace mixer_client { +namespace { + +const char kCheckRequest1[] = R"( +request_index: 1 +attribute_update { + string_attributes { + key: 1 + value: "service_name" + } + string_attributes { + key: 2 + value: "operation_name" + } + int64_attributes { + key: 1 + value: 400 + } +} +)"; + +const char kReportRequest1[] = R"( +request_index: 1 +attribute_update { + string_attributes { + key: 1 + value: "service_name" + } + string_attributes { + key: 2 + value: "operation_name" + } + int64_attributes { + key: 1 + value: 400 + } +} +)"; + +const char kQuotaRequest1[] = R"( +request_index: 1 +attribute_update { + string_attributes { + key: 1 + value: "service_name" + } + string_attributes { + key: 2 + value: "operation_name" + } + int64_attributes { + key: 1 + value: 400 + } +} +)"; +const char kSuccessCheckResponse1[] = R"( +request_index: 1 +result { + code: 200 +} +)"; + +const char kReportResponse1[] = R"( +request_index: 1 +result { + code: 200 +} +)"; + +const char kQuotaResponse1[] = R"( +request_index: 1 +)"; + +const char kErrorCheckResponse1[] = R"()"; + +// A mocking class to mock CheckTransport interface. +class MockCheckTransport { + public: + MOCK_METHOD3(Check, void(const CheckRequest &, CheckResponse *, DoneFunc)); + + TransportCheckFunc GetFunc() { + return + [this](const CheckRequest &request, CheckResponse *response, + DoneFunc on_done) { this->Check(request, response, on_done); }; + } + + MockCheckTransport() : check_response_(NULL) {} + + ~MockCheckTransport() {} + + // The done callback is called right away (in place). + void CheckWithInplaceCallback(const CheckRequest &request, + CheckResponse *response, DoneFunc on_done) { + check_request_ = request; + if (check_response_) { + *response = *check_response_; + } + on_done(done_status_); + } + + // Saved check_request from mocked Transport::Check() call. + CheckRequest check_request_; + // If not NULL, the check response to send for mocked Transport::Check() call. + CheckResponse *check_response_; + + // The status to send in on_done call back for Check() or Report(). + Status done_status_; +}; + +// A mocking class to mock ReportTransport interface. +class MockReportTransport { + public: + MOCK_METHOD3(Report, void(const ReportRequest &, ReportResponse *, DoneFunc)); + + TransportReportFunc GetFunc() { + return + [this](const ReportRequest &request, ReportResponse *response, + DoneFunc on_done) { this->Report(request, response, on_done); }; + } + + MockReportTransport() : report_response_(NULL) {} + + ~MockReportTransport() {} + + // The done callback is called right away (in place). + void ReportWithInplaceCallback(const ReportRequest &request, + ReportResponse *response, DoneFunc on_done) { + report_request_ = request; + if (report_response_) { + *response = *report_response_; + } + on_done(done_status_); + } + + // Saved report_request from mocked Transport::Report() call. + ReportRequest report_request_; + // If not NULL, the report response to send for mocked Transport::Report() + // call. + ReportResponse *report_response_; + + // The status to send in on_done call back for Check() or Report(). + Status done_status_; +}; + +// A mocking class to mock QuotaTransport interface. +class MockQuotaTransport { + public: + MOCK_METHOD3(Quota, void(const QuotaRequest &, QuotaResponse *, DoneFunc)); + + TransportQuotaFunc GetFunc() { + return + [this](const QuotaRequest &request, QuotaResponse *response, + DoneFunc on_done) { this->Quota(request, response, on_done); }; + } + + MockQuotaTransport() : quota_response_(NULL) {} + + ~MockQuotaTransport() {} + + // The done callback is called right away (in place). + void QuotaWithInplaceCallback(const QuotaRequest &request, + QuotaResponse *response, DoneFunc on_done) { + quota_request_ = request; + if (quota_response_) { + *response = *quota_response_; + } + on_done(done_status_); + } + + // Saved report_request from mocked Transport::Report() call. + QuotaRequest quota_request_; + // If not NULL, the report response to send for mocked Transport::Report() + // call. + QuotaResponse *quota_response_; + + // The status to send in on_done call back for Check() or Report(). + Status done_status_; +}; +} // namespace + +class MixerClientImplTest : public ::testing::Test { + public: + void SetUp() { + ASSERT_TRUE(TextFormat::ParseFromString(kCheckRequest1, &check_request1_)); + ASSERT_TRUE(TextFormat::ParseFromString(kSuccessCheckResponse1, + &pass_check_response1_)); + ASSERT_TRUE( + TextFormat::ParseFromString(kReportRequest1, &report_request1_)); + + ASSERT_TRUE(TextFormat::ParseFromString(kQuotaRequest1, "a_request1_)); + + MixerClientOptions options( + CheckOptions(1 /*entries */, 500 /* refresh_interval_ms */, + 1000 /* expiration_ms */), + ReportOptions(1 /* entries */, 500 /*flush_interval_ms*/), + QuotaOptions(1 /* entries */, 500 /*flush_interval_ms*/, + 1000 /* expiration_ms */)); + options.check_transport = mock_check_transport_.GetFunc(); + options.report_transport = mock_report_transport_.GetFunc(); + options.quota_transport = mock_quota_transport_.GetFunc(); + client_ = CreateMixerClient(options); + } + + CheckRequest check_request1_; + CheckResponse pass_check_response1_; + CheckResponse error_check_response1_; + + ReportRequest report_request1_; + QuotaRequest quota_request1_; + + MockCheckTransport mock_check_transport_; + MockReportTransport mock_report_transport_; + MockQuotaTransport mock_quota_transport_; + + std::unique_ptr client_; +}; + +TEST_F(MixerClientImplTest, TestNonCachedCheckWithInplaceCallback) { + // Calls a Client::Check, the request is not in the cache + // Transport::Check() is called. It will send a successful check response + EXPECT_CALL(mock_check_transport_, Check(_, _, _)) + .WillOnce(Invoke(&mock_check_transport_, + &MockCheckTransport::CheckWithInplaceCallback)); + + mock_check_transport_.done_status_ = Status::OK; + + CheckResponse check_response; + Status done_status = Status::UNKNOWN; + client_->Check(check_request1_, &check_response, + [&done_status](Status status) { done_status = status; }); + + // Since it is not cached, transport should be called. + EXPECT_TRUE(MessageDifferencer::Equals(mock_check_transport_.check_request_, + check_request1_)); +} + +TEST_F(MixerClientImplTest, TestNonCachedReportWithInplaceCallback) { + // Calls Client::Report, it will not be cached. + // Transport::Report() should be called. + // Transport::on_done() is called inside Transport::Report() with error + // PERMISSION_DENIED. The Client::done_done() is called with the same error. + EXPECT_CALL(mock_report_transport_, Report(_, _, _)) + .WillOnce(Invoke(&mock_report_transport_, + &MockReportTransport::ReportWithInplaceCallback)); + + mock_report_transport_.done_status_ = Status(Code::PERMISSION_DENIED, ""); + + ReportResponse report_response; + Status done_status = Status::UNKNOWN; + // This request is high important, so it will not be cached. + // client->Report() will call Transport::Report() right away. + client_->Report(report_request1_, &report_response, + [&done_status](Status status) { done_status = status; }); + + // Since it is not cached, transport should be called. + EXPECT_TRUE(MessageDifferencer::Equals(mock_report_transport_.report_request_, + report_request1_)); +} + +TEST_F(MixerClientImplTest, TestNonCachedQuotaWithInplaceCallback) { + // Calls Client::Quota with a high important request, it will not be cached. + // Transport::Quota() should be called. + EXPECT_CALL(mock_quota_transport_, Quota(_, _, _)) + .WillOnce(Invoke(&mock_quota_transport_, + &MockQuotaTransport::QuotaWithInplaceCallback)); + + // Set the report status to be used in the on_report_done + mock_report_transport_.done_status_ = Status::OK; + + QuotaResponse quota_response; + Status done_status = Status::UNKNOWN; + client_->Quota(quota_request1_, "a_response, + [&done_status](Status status) { done_status = status; }); + + // Since it is not cached, transport should be called. + EXPECT_TRUE(MessageDifferencer::Equals(mock_quota_transport_.quota_request_, + quota_request1_)); +} + +} // namespace mixer_client +} // namespace istio \ No newline at end of file From 5d9142d03718499eb05bb5731c235b507d76fc3a Mon Sep 17 00:00:00 2001 From: Qian Sun Date: Wed, 11 Jan 2017 17:43:50 -0800 Subject: [PATCH 2/2] Fix bug and update travis.yml. --- mixerclient/.travis.yml | 2 +- mixerclient/BUILD | 2 +- mixerclient/WORKSPACE | 2 +- mixerclient/src/client_impl.h | 2 +- mixerclient/src/client_impl_test.cc | 6 ++++-- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/mixerclient/.travis.yml b/mixerclient/.travis.yml index 127b46cde016..f51cae6d40d2 100644 --- a/mixerclient/.travis.yml +++ b/mixerclient/.travis.yml @@ -24,4 +24,4 @@ install: script: - script/check-style - - bazel build :all + - bazel test :all --test_output=errors diff --git a/mixerclient/BUILD b/mixerclient/BUILD index 0472f4947de5..47d1c7d6d553 100644 --- a/mixerclient/BUILD +++ b/mixerclient/BUILD @@ -42,4 +42,4 @@ cc_test( ":mixer_client_lib", "//external:googletest_main", ], -) \ No newline at end of file +) diff --git a/mixerclient/WORKSPACE b/mixerclient/WORKSPACE index d48f97f53629..16b03d42527a 100644 --- a/mixerclient/WORKSPACE +++ b/mixerclient/WORKSPACE @@ -94,4 +94,4 @@ bind( bind( name = "googletest_main", actual = "@googletest_git//:googletest_main", -) \ No newline at end of file +) diff --git a/mixerclient/src/client_impl.h b/mixerclient/src/client_impl.h index 0aca1fa1881b..fae68f06584f 100644 --- a/mixerclient/src/client_impl.h +++ b/mixerclient/src/client_impl.h @@ -52,7 +52,7 @@ class MixerClientImpl : public MixerClient { DoneFunc on_quota_done); private: - const MixerClientOptions &options_; + MixerClientOptions options_; GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(MixerClientImpl); }; diff --git a/mixerclient/src/client_impl_test.cc b/mixerclient/src/client_impl_test.cc index 68c3a3d2cdf4..945acaa3b019 100644 --- a/mixerclient/src/client_impl_test.cc +++ b/mixerclient/src/client_impl_test.cc @@ -15,7 +15,6 @@ #include "include/client.h" -#include #include "gmock/gmock.h" #include "google/protobuf/text_format.h" #include "google/protobuf/util/message_differencer.h" @@ -266,6 +265,7 @@ TEST_F(MixerClientImplTest, TestNonCachedCheckWithInplaceCallback) { Status done_status = Status::UNKNOWN; client_->Check(check_request1_, &check_response, [&done_status](Status status) { done_status = status; }); + EXPECT_EQ(done_status, mock_check_transport_.done_status_); // Since it is not cached, transport should be called. EXPECT_TRUE(MessageDifferencer::Equals(mock_check_transport_.check_request_, @@ -289,6 +289,7 @@ TEST_F(MixerClientImplTest, TestNonCachedReportWithInplaceCallback) { // client->Report() will call Transport::Report() right away. client_->Report(report_request1_, &report_response, [&done_status](Status status) { done_status = status; }); + EXPECT_EQ(done_status, mock_report_transport_.done_status_); // Since it is not cached, transport should be called. EXPECT_TRUE(MessageDifferencer::Equals(mock_report_transport_.report_request_, @@ -309,6 +310,7 @@ TEST_F(MixerClientImplTest, TestNonCachedQuotaWithInplaceCallback) { Status done_status = Status::UNKNOWN; client_->Quota(quota_request1_, "a_response, [&done_status](Status status) { done_status = status; }); + EXPECT_EQ(done_status, mock_quota_transport_.done_status_); // Since it is not cached, transport should be called. EXPECT_TRUE(MessageDifferencer::Equals(mock_quota_transport_.quota_request_, @@ -316,4 +318,4 @@ TEST_F(MixerClientImplTest, TestNonCachedQuotaWithInplaceCallback) { } } // namespace mixer_client -} // namespace istio \ No newline at end of file +} // namespace istio