You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/04/28 23:04:49 UTC
[GitHub] [geode] alb3rtobr opened a new pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
alb3rtobr opened a new pull request #6393:
URL: https://github.com/apache/geode/pull/6393
`testCheckerChecksValidAttribute` in `DistributionConfigJUnitTest` has a "TODO" comment about a missing check in the test.
I have also changed JUnit asserts by AssertJ asserts, which are more descriptives in case of error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] alb3rtobr commented on pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6393:
URL: https://github.com/apache/geode/pull/6393#issuecomment-848711241
Kind reminder, review by code owners still pending
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6393:
URL: https://github.com/apache/geode/pull/6393#discussion_r658183997
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -138,19 +135,19 @@ public void testGetAttributeNames() {
// TODO - This makes no sense. One has no idea what the correct expected number of attributes
// are.
- assertEquals(36, boolList.size());
- assertEquals(35, intList.size());
- assertEquals(88, stringList.size());
- assertEquals(5, fileList.size());
- assertEquals(5, otherList.size());
+ assertThat(boolList).hasSize(36);
+ assertThat(intList).hasSize(35);
+ assertThat(stringList).hasSize(88);
+ assertThat(fileList).hasSize(5);
+ assertThat(otherList).hasSize(5);
}
@Test
public void testAttributeDesc() {
String[] attNames = AbstractDistributionConfig._getAttNames();
for (String attName : attNames) {
- assertTrue("Does not contain description for attribute " + attName,
- AbstractDistributionConfig.dcAttDescriptions.containsKey(attName));
+ assertThat(AbstractDistributionConfig.dcAttDescriptions.containsKey(attName))
+ .as("Does not contain description for attribute " + attName).isTrue();
Review comment:
Many assertions still need further change after using the "Convert Assertions to AssertJ" IntelliJ plugin. Assertions on collections usually require some further help.
Let's change this assertion to:
```
assertThat(AbstractDistributionConfig.dcAttDescriptions).containsKey(attName);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] alb3rtobr merged pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
Posted by GitBox <gi...@apache.org>.
alb3rtobr merged pull request #6393:
URL: https://github.com/apache/geode/pull/6393
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #6393:
URL: https://github.com/apache/geode/pull/6393#issuecomment-832888170
PR checks in this PR are now a week old (and don't include DistributedTestOpenJDK8). If possible @alb3rtobr could you please push an empty commit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] alb3rtobr commented on a change in pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #6393:
URL: https://github.com/apache/geode/pull/6393#discussion_r650297169
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -235,55 +232,59 @@ public void everyGetterSetterSameNameSameType() {
for (String attr : getters.keySet()) {
Method getter = getters.get(attr);
Method setter = setters.get(attr);
- assertNotNull("every getter should have a corresponding setter " + attr, setter);
+ assertThat(setter).as("every getter should have a corresponding setter " + attr).isNotNull();
String setterName = setter.getName();
String getterName = getter.getName();
- assertEquals(setterName.substring(setterName.indexOf("set") + 3),
- getterName.substring(getterName.indexOf("get") + 3));
- assertEquals(setter.getParameterTypes()[0], getter.getReturnType());
+ assertThat(getterName.substring(getterName.indexOf("get") + 3))
+ .isEqualTo(setterName.substring(setterName.indexOf("set") + 3));
+ assertThat(getter.getReturnType()).isEqualTo(setter.getParameterTypes()[0]);
Review comment:
I have refactored the assertion about the setter and getter names as:
```
String attrNameInGetterSignature = getter.getName().substring(3);
assertThat(setter.getName()).contains(attrNameInGetterSignature);
```
I think now its easier to understand what it is being checked there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6393:
URL: https://github.com/apache/geode/pull/6393#discussion_r650148533
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -162,17 +159,17 @@ public void testAttributeDesc() {
@Test
public void sameCount() {
- assertEquals(attributes.size(), setters.size());
- assertEquals(setters.size(), getters.size());
+ assertThat(setters).hasSize(attributes.size());
+ assertThat(getters).hasSize(setters.size());
Review comment:
There is a `hasSameSizeAs` assertion that is cleaner and provides better failure messages:
```
assertThat(setters).hasSameSizeAs(attributes);
assertThat(getters).hasSameSizeAs(setters);
```
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -235,55 +232,59 @@ public void everyGetterSetterSameNameSameType() {
for (String attr : getters.keySet()) {
Method getter = getters.get(attr);
Method setter = setters.get(attr);
- assertNotNull("every getter should have a corresponding setter " + attr, setter);
+ assertThat(setter).as("every getter should have a corresponding setter " + attr).isNotNull();
String setterName = setter.getName();
String getterName = getter.getName();
- assertEquals(setterName.substring(setterName.indexOf("set") + 3),
- getterName.substring(getterName.indexOf("get") + 3));
- assertEquals(setter.getParameterTypes()[0], getter.getReturnType());
+ assertThat(getterName.substring(getterName.indexOf("get") + 3))
+ .isEqualTo(setterName.substring(setterName.indexOf("set") + 3));
+ assertThat(getter.getReturnType()).isEqualTo(setter.getParameterTypes()[0]);
}
for (String attr : setters.keySet()) {
Method getter = getters.get(attr);
- assertNotNull("every setter should have a corresponding getter: " + attr, getter);
+ assertThat(getter).as("every setter should have a corresponding getter: " + attr).isNotNull();
}
}
@Test
public void everySetterHasAttributeDefined() {
for (String attr : setters.keySet()) {
ConfigAttribute configAttribute = attributes.get(attr);
- assertNotNull(attr + " should be defined a ConfigAttribute", configAttribute);
+ assertThat(configAttribute).as(attr + " should be defined a ConfigAttribute").isNotNull();
}
}
@Test
public void everyGetterHasAttributeDefined() {
for (String attr : getters.keySet()) {
ConfigAttribute configAttribute = attributes.get(attr);
- assertNotNull(attr + " should be defined a ConfigAttribute", configAttribute);
+ assertThat(configAttribute).as(attr + " should be defined a ConfigAttribute").isNotNull();
}
}
@Test
public void testGetAttributeObject() {
- assertEquals(config.getAttributeObject(LOG_LEVEL), "config");
- assertEquals(config.getAttributeObject(SECURITY_LOG_LEVEL), "config");
- assertEquals(config.getAttributeObject(REDUNDANCY_ZONE), "");
- assertEquals(config.getAttributeObject(ENABLE_CLUSTER_CONFIGURATION).getClass(), Boolean.class);
+ assertThat(config.getAttributeObject(LOG_LEVEL)).isEqualTo("config");
+ assertThat(config.getAttributeObject(SECURITY_LOG_LEVEL)).isEqualTo("config");
+ assertThat(config.getAttributeObject(REDUNDANCY_ZONE)).isEqualTo("");
+ assertThat(config.getAttributeObject(ENABLE_CLUSTER_CONFIGURATION).getClass())
+ .isEqualTo(Boolean.class);
Review comment:
Better assertion syntax is:
```
assertThat(config.getAttributeObject(ENABLE_CLUSTER_CONFIGURATION)).isInstanceOf(Boolean.class);
```
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -235,55 +232,59 @@ public void everyGetterSetterSameNameSameType() {
for (String attr : getters.keySet()) {
Method getter = getters.get(attr);
Method setter = setters.get(attr);
- assertNotNull("every getter should have a corresponding setter " + attr, setter);
+ assertThat(setter).as("every getter should have a corresponding setter " + attr).isNotNull();
String setterName = setter.getName();
String getterName = getter.getName();
- assertEquals(setterName.substring(setterName.indexOf("set") + 3),
- getterName.substring(getterName.indexOf("get") + 3));
- assertEquals(setter.getParameterTypes()[0], getter.getReturnType());
+ assertThat(getterName.substring(getterName.indexOf("get") + 3))
+ .isEqualTo(setterName.substring(setterName.indexOf("set") + 3));
+ assertThat(getter.getReturnType()).isEqualTo(setter.getParameterTypes()[0]);
Review comment:
Another String assertion syntax that works better is:
```
assertThat("foobar").contains("foo");
```
I would lean away from using`substring` and `indexOf` within assertions.
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -162,17 +159,17 @@ public void testAttributeDesc() {
@Test
public void sameCount() {
- assertEquals(attributes.size(), setters.size());
- assertEquals(setters.size(), getters.size());
+ assertThat(setters).hasSize(attributes.size());
+ assertThat(getters).hasSize(setters.size());
}
@Test
public void everyAttrHasValidSetter() {
for (String attr : attributes.keySet()) {
Method setter = setters.get(attr);
- assertNotNull(attr + " should have a setter", setter);
- assertTrue(setter.getName().startsWith("set"));
- assertEquals(setter.getParameterCount(), 1);
+ assertThat(setter).as(attr + " should have a setter").isNotNull();
+ assertThat(setter.getName().startsWith("set")).isTrue();
Review comment:
A better way to form this assertion with better failure message:
```
assertThat(setter.getName()).startsWith("set");
```
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -235,55 +232,59 @@ public void everyGetterSetterSameNameSameType() {
for (String attr : getters.keySet()) {
Method getter = getters.get(attr);
Method setter = setters.get(attr);
- assertNotNull("every getter should have a corresponding setter " + attr, setter);
+ assertThat(setter).as("every getter should have a corresponding setter " + attr).isNotNull();
String setterName = setter.getName();
String getterName = getter.getName();
- assertEquals(setterName.substring(setterName.indexOf("set") + 3),
- getterName.substring(getterName.indexOf("get") + 3));
- assertEquals(setter.getParameterTypes()[0], getter.getReturnType());
+ assertThat(getterName.substring(getterName.indexOf("get") + 3))
+ .isEqualTo(setterName.substring(setterName.indexOf("set") + 3));
+ assertThat(getter.getReturnType()).isEqualTo(setter.getParameterTypes()[0]);
}
for (String attr : setters.keySet()) {
Method getter = getters.get(attr);
- assertNotNull("every setter should have a corresponding getter: " + attr, getter);
+ assertThat(getter).as("every setter should have a corresponding getter: " + attr).isNotNull();
}
}
@Test
public void everySetterHasAttributeDefined() {
for (String attr : setters.keySet()) {
ConfigAttribute configAttribute = attributes.get(attr);
- assertNotNull(attr + " should be defined a ConfigAttribute", configAttribute);
+ assertThat(configAttribute).as(attr + " should be defined a ConfigAttribute").isNotNull();
}
}
@Test
public void everyGetterHasAttributeDefined() {
for (String attr : getters.keySet()) {
ConfigAttribute configAttribute = attributes.get(attr);
- assertNotNull(attr + " should be defined a ConfigAttribute", configAttribute);
+ assertThat(configAttribute).as(attr + " should be defined a ConfigAttribute").isNotNull();
}
}
@Test
public void testGetAttributeObject() {
- assertEquals(config.getAttributeObject(LOG_LEVEL), "config");
- assertEquals(config.getAttributeObject(SECURITY_LOG_LEVEL), "config");
- assertEquals(config.getAttributeObject(REDUNDANCY_ZONE), "");
- assertEquals(config.getAttributeObject(ENABLE_CLUSTER_CONFIGURATION).getClass(), Boolean.class);
+ assertThat(config.getAttributeObject(LOG_LEVEL)).isEqualTo("config");
+ assertThat(config.getAttributeObject(SECURITY_LOG_LEVEL)).isEqualTo("config");
+ assertThat(config.getAttributeObject(REDUNDANCY_ZONE)).isEqualTo("");
+ assertThat(config.getAttributeObject(ENABLE_CLUSTER_CONFIGURATION).getClass())
+ .isEqualTo(Boolean.class);
}
@Test
public void testCheckerChecksValidAttribute() {
for (String att : checkers.keySet()) {
System.out.println("att = " + att);
- assertTrue(attributes.containsKey(att));
+ assertThat(attributes.containsKey(att)).isTrue();
Review comment:
Recommend:
```
assertThat(attributes).containsKey(att);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] alb3rtobr commented on pull request #6393: GEODE-9209: Complete test in DistributionConfigJUnitTest
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6393:
URL: https://github.com/apache/geode/pull/6393#issuecomment-859943318
Thanks for the review @kirklund , I have implemented the requested changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org