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 390/rm some expected fail #302

Merged
merged 3 commits into from Sep 26, 2017

Conversation

rigazilla
Copy link
Contributor

This PR contains changes on the swig wrapper that remove some of expected failure in the java test list.
Plus a fix on the client that now correctly set the force return value flag in the request.

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

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.

Just some formatting issues in the PR comments, but I am seeing these tests fail:

The following tests FAILED:
	 16 - simpleSasl (OTHER_FAULT)
	 22 - simple-tls (OTHER_FAULT)
	 23 - simple-tls-sni (Failed)
	 28 - simple-tls-client-auth (OTHER_FAULT)
	 29 - simple-tls-sni-client-auth (Failed)

this.marshaller = marshaller2;
this.servers = null;
this.failoverServers = null;
this.transportFactory = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation looks wrong on this line

@@ -290,8 +289,9 @@ public Configuration create() {

@Override
public Configuration build() {
return new Configuration(this.jniConfigurationBuilder.build());
}
return (marshaller != null) ? new Configuration(this.jniConfigurationBuilder.build(), marshaller)
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation looks wrong on these lines

@@ -78,19 +78,35 @@ public RemoteCacheManager(Properties props, boolean start) {
}

public <K, V> org.infinispan.client.hotrod.RemoteCache<K, V> getCache() {
return new org.infinispan.client.hotrod.impl.RemoteCacheImpl<K, V>(this, "", false);
try {
return new org.infinispan.client.hotrod.impl.RemoteCacheImpl<K, V>(this, "", false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation look wrong in all four of these methods

@rigazilla rigazilla force-pushed the HRCPP-390/rm_some_expected_fail branch from 2288b1f to 4e44015 Compare September 15, 2017 14:17
@rigazilla
Copy link
Contributor Author

I've removed the tabs.

It seems that you had an infinispan server already running before you run the test suite. This conflicts with the TLS and SASL startup.

@rigazilla
Copy link
Contributor Author

adding @mgencur as reviewer since he's the reporter of this issue.

@alanfx
Copy link
Contributor

alanfx commented Sep 15, 2017

@rigazilla ok, code and tests look good to me now. 👍

Copy link
Contributor

@mgencur mgencur left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rigazilla
Copy link
Contributor Author

@alanfx if it's all ok can you merge? thanx!

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 just some minor nits and a question. The tests are passing on my machine, though.

@@ -17,19 +17,19 @@
import org.testng.reporters.TextReporter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed

@@ -158,18 +155,17 @@ when main() returns. */

@Override
public boolean includeMethod(IMethodSelectorContext context, ITestNGMethod method, boolean isTestMethod) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the indentation for includeMethod and setTestMethods is off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed

@@ -178,9 +178,8 @@ public ConfigurationBuilder marshaller(Class<? extends Marshaller> marshaller) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this^^^ version of the method work too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok enabled

@alanfx
Copy link
Contributor

alanfx commented Sep 26, 2017

Looks good to me. Merging...

@alanfx alanfx merged commit 0c83c98 into infinispan:master Sep 26, 2017
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

3 participants