Skip to content
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

Hrcpp 402/near cache gtest #306

Merged
merged 1 commit into from Oct 13, 2017

Conversation

rigazilla
Copy link
Contributor

near cache xunit test suite

https://issues.jboss.org/browse/HRCPP-402

@rigazilla rigazilla force-pushed the HRCPP-402/near_cache_gtest branch 2 times, most recently from 57335f5 to de92703 Compare September 26, 2017 21:51
@rigazilla rigazilla force-pushed the HRCPP-402/near_cache_gtest branch 2 times, most recently from 726c0ad to fa2c741 Compare October 5, 2017 16:55
Copy link
Contributor

@alanfx alanfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rigazilla there are some formatting issues in a lot of the files. I can also see that the tests in NearCacheStaleReadsTest map to the tests in the "client.hotrod.near.AvoidStaleNearCacheReadsTest" class in the Java test suite. I'm not sure which Java class the tests in NearCacheMultiClientTest map to. It also seems like there are some other tests from the Java test suite that haven't been implemented here? (i.e. eviction, failover, etc.)

@@ -0,0 +1,158 @@
package sample_bank_account;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid messages like this during the build:

[libprotobuf WARNING google/protobuf/compiler/parser.cc:546] No syntax specified for the proto file: bank-xunit.proto. Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version. (Defaulted to proto2 syntax.)

The proto files should contain a line like: syntax = "proto3"; or syntax = "proto2";.


void NearCacheTest::SetUp() {
if (NearCacheTest::remoteCacheManager == nullptr){
ConfigurationBuilder builder;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation seems off?


TEST_F(NearCacheTest, ClientsInvalidatedTest) {
NearCacheTest::remoteCacheManager->start();
BasicMarshaller<std::string> *km1 = new BasicMarshaller<std::string>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation seems off?


void NearCacheStaleReadsTest::SetUp() {
if (NearCacheStaleReadsTest::remoteCacheManager == nullptr){
ConfigurationBuilder builder;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also indentation issues in this file

std::unique_ptr<infinispan::hotrod::RemoteCacheManager> NearCacheTest::remoteCacheManager;

void waitFor(const std::function<bool()> &func, long waitTime = 1000, int pollInterval = 100);
/*void waitFor(const std::function<bool()> &func, long waitTime = 1000, int pollInterval = 100)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?

@@ -0,0 +1,31 @@
file(GLOB SRCS *.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the QueryTest changes be in this PR or in #305?

@rigazilla
Copy link
Contributor Author

@alanfx cleaned up and rebased to not contain query tests.
NearCacheMultiClientTestSuite is based on the relative C# class. Can we track the missing test on a separated jira so we can have the xunit structure merged?

@alanfx
Copy link
Contributor

alanfx commented Oct 13, 2017

@rigazilla actually, I'd just like to keep https://issues.jboss.org/browse/HRCPP-402 open and add comments there. I'm not sure if the failover tests will get implemented, but the eviction ones should. The changes in this PR look good to me, so let me know if that works for you.

@rigazilla
Copy link
Contributor Author

rigazilla commented Oct 13, 2017

@alanfx it's just because this will conflict with the #305 so I would like to have one of the two merged and the other rebased as soon as possible, to simplify the PRs. But it's not a big problem

@alanfx alanfx merged commit d23af70 into infinispan:master Oct 13, 2017
@alanfx
Copy link
Contributor

alanfx commented Oct 13, 2017

@rigazilla No problem. I have merged this PR, and added a comment to the JIRA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants