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/10/06 23:38:36 UTC
[GitHub] [geode] nabarunnag opened a new pull request #6950: GEODE-9686: Remove deprecated elements from GemfireDataCommandsDistri…
nabarunnag opened a new pull request #6950:
URL: https://github.com/apache/geode/pull/6950
…butedTestBase
* Deprecated elements were removed
* method overrides were fixed.
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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] kirklund commented on a change in pull request #6950: GEODE-9686: Remove deprecated elements from GemfireDataCommandsDistri…
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6950:
URL: https://github.com/apache/geode/pull/6950#discussion_r725170994
##########
File path: geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/GeodeDataCommandsDistributedTestBase.java
##########
@@ -94,47 +87,35 @@
protected MemberVM locator, server1, server2;
- public void before() throws Exception {
- locator = cluster.startLocatorVM(0, locatorProperties());
- server1 = cluster.startServerVM(1, locator.getPort());
- server2 = cluster.startServerVM(2, locator.getPort());
- connectToLocator();
- }
-
- // extracted for convenience overriding in GemfireDataCommandsOverHttpDUnitTest to connect via
- // http
public void connectToLocator() throws Exception {
gfsh.connectAndVerify(locator);
}
private static void setupRegions(String regionName, String parRegionName) {
InternalCache cache = ClusterStartupRule.getCache();
- RegionFactory regionFactory =
+ assert cache != null;
Review comment:
[comment] I realize this is fairly subjective, so it's just a comment.
One best practice for tests is to never use Java `assert` and only use `assertThat` assertions.
Another best practice for tests is to never perform null checks and just let the test code throw NPE if something is null unless the main purpose of the test is to validate that something is not null (which would generally involve an assertion at or near the end of the test).
I think this and all of the following `assertThat(xxx).isNotNull()` lines are really just to make one optional inspection in IntelliJ happy. This inspection is recommended for Production code, but you can see the noise problem it causes for Test code.
There are a total of 14 of these lines in the develop version of this class and everyone of these is generally considered a bad practice in JUnit tests:
```
import static org.apache.geode.test.dunit.Assert.assertNotNull;
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataParRegion);
assertNotNull(dataParRegion);
assertNotNull(service.getMemberMXBean());
assertNotNull(service.getManagerMXBean());
assertNotNull(bean);
assertNotNull(bean);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
```
I recommend deleting all null assertions and following any these approaches instead:
1. In IntelliJ, go to Editor -> Inspections, search for `Constant conditions & exceptions` and change its scope to Product only (not Tests). Unfortunately we have test base classes like `GemfireDataCommandsDUnitTestBase` in the src/main which IntelliJ considers Product code so that doesn't work in this case.
2. Suppress this warning for this test class with `@SuppressWarnings("ConstantConditions")`
3. Do nothing and just let the code have warnings. We can't suppress or fix every single inspection in IntellIj. Some of the inspections should _not_ be universally applied blindly to all code (such as in this case), some of the inspections are only supposed to be used on the fly to search for certain things in the code base, some of them are actually binary opposites each asking you to do the opposite of the other.
Many of us have different IntelliJ Inspections enabled/disabled/customized. Unless we pick a standard Inspections configuration file to use for Geode like we do with Spotless, this is going to be messy and contentious.
In general, I support correcting all real warnings in both product and test code, but adding lots of null assertions for this Inspection probably makes the test code worse.
--
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] kirklund commented on a change in pull request #6950: GEODE-9686: Remove deprecated elements from GemfireDataCommandsDistri…
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6950:
URL: https://github.com/apache/geode/pull/6950#discussion_r725170994
##########
File path: geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/GeodeDataCommandsDistributedTestBase.java
##########
@@ -94,47 +87,35 @@
protected MemberVM locator, server1, server2;
- public void before() throws Exception {
- locator = cluster.startLocatorVM(0, locatorProperties());
- server1 = cluster.startServerVM(1, locator.getPort());
- server2 = cluster.startServerVM(2, locator.getPort());
- connectToLocator();
- }
-
- // extracted for convenience overriding in GemfireDataCommandsOverHttpDUnitTest to connect via
- // http
public void connectToLocator() throws Exception {
gfsh.connectAndVerify(locator);
}
private static void setupRegions(String regionName, String parRegionName) {
InternalCache cache = ClusterStartupRule.getCache();
- RegionFactory regionFactory =
+ assert cache != null;
Review comment:
[comment] I realize this is fairly subjective, so it's just a comment.
One best practice for tests is to never use Java `assert` and only use `assertThat` assertions.
Another best practice for tests is to never perform null checks and just let the test code throw NPE if something is null unless the main purpose of the test is to validate that something is not null (which would generally involve an assertion at or near the end of the test).
I think this and all of the following `assertThat(xxx).isNotNull()` lines are really just to make one optional inspection in IntelliJ happy. This inspection is recommended for Production code, but you can see the noise problem it causes for Test code.
There are a total of 14 of these lines in the develop version of this class and everyone of these is generally considered a bad practice in JUnit tests:
```
import static org.apache.geode.test.dunit.Assert.assertNotNull;
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataParRegion);
assertNotNull(dataParRegion);
assertNotNull(service.getMemberMXBean());
assertNotNull(service.getManagerMXBean());
assertNotNull(bean);
assertNotNull(bean);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
```
I recommend deleting all null assertions and following any these approaches instead:
1. In IntelliJ, go to Editor -> Inspections, search for `Constant conditions & exceptions` and change its scope to Product only (not Tests). Unfortunately we have test base classes like `GemfireDataCommandsDUnitTestBase` in the src/main which IntelliJ considers Product code so that doesn't work in this case.
2. Suppress this warning for this test class with `@SuppressWarnings("ConstantConditions")`
3. Do nothing and just let the code have warnings. We can't suppress or fix every single inspection in IntellIj. Some of the inspections should [edit] _not_ be universally applied blindly to all code (such as in this case), some of the inspections are only supposed to be used on the fly to search for certain things in the code base, some of them are actually binary opposites each asking you to do the opposite of the other.
Many of us have different IntelliJ Inspections enabled/disabled/customized. Unless we pick a standard Inspections configuration file to use for Geode like we do with Spotless, this is going to be messy and contentious.
In general, I support correcting all real warnings in both product and test code, but adding lots of null assertions for this Inspection probably makes the test code worse.
--
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] kirklund commented on a change in pull request #6950: GEODE-9686: Remove deprecated elements from GemfireDataCommandsDistri…
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6950:
URL: https://github.com/apache/geode/pull/6950#discussion_r725170994
##########
File path: geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/GeodeDataCommandsDistributedTestBase.java
##########
@@ -94,47 +87,35 @@
protected MemberVM locator, server1, server2;
- public void before() throws Exception {
- locator = cluster.startLocatorVM(0, locatorProperties());
- server1 = cluster.startServerVM(1, locator.getPort());
- server2 = cluster.startServerVM(2, locator.getPort());
- connectToLocator();
- }
-
- // extracted for convenience overriding in GemfireDataCommandsOverHttpDUnitTest to connect via
- // http
public void connectToLocator() throws Exception {
gfsh.connectAndVerify(locator);
}
private static void setupRegions(String regionName, String parRegionName) {
InternalCache cache = ClusterStartupRule.getCache();
- RegionFactory regionFactory =
+ assert cache != null;
Review comment:
[comment] I realize this is fairly subjective, so it's just a comment.
One best practice for tests is to never use Java `assert` and only use `assertThat` assertions.
Another best practice for tests is to never perform null checks and just let the test code throw NPE if something is null unless the main purpose of the test is to validate that something is not null (which would generally involve an assertion at or near the end of the test).
I think this and all of the following `assertThat(xxx).isNotNull()` lines are really just to make one optional inspection in IntelliJ happy. This inspection is recommended for Production code, but you can see the noise problem it causes for Test code.
There are a total of 14 of these lines in the develop version of this class and everyone of these is generally considered a bad practice in JUnit tests:
```
import static org.apache.geode.test.dunit.Assert.assertNotNull;
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataParRegion);
assertNotNull(dataParRegion);
assertNotNull(service.getMemberMXBean());
assertNotNull(service.getManagerMXBean());
assertNotNull(bean);
assertNotNull(bean);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
```
I recommend deleting all null assertions and following any these approaches instead:
1. In IntelliJ, go to Editor -> Inspections, search for `Constant conditions & exceptions` and change its scope to Product only (not Tests). Unfortunately we have test base classes like `GemfireDataCommandsDUnitTestBase` in the src/main which IntelliJ considers Product code so that doesn't work in this case.
2. Suppress this warning for this test class with `@SuppressWarnings("ConstantConditions")`
3. Do nothing and just let the code have warnings. We can't suppress or fix every single inspection in IntellIj. Some of the inspections should _not_ be universally applied blindly to all code (such as in this case), some of the inspections are only supposed to be used on the fly to search for certain things in the code base, some of them are actually binary opposites (two different inspections that are the opposite of the other).
Many of us have different IntelliJ Inspections enabled/disabled/customized. Unless we pick a standard Inspections configuration file to use for Geode like we do with Spotless, this is going to be messy and contentious.
In general, I support correcting all real warnings in both product and test code, but adding lots of null assertions for this Inspection probably makes the test code worse.
--
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] kirklund commented on a change in pull request #6950: GEODE-9686: Remove deprecated elements from GemfireDataCommandsDistri…
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6950:
URL: https://github.com/apache/geode/pull/6950#discussion_r725192876
##########
File path: geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/GeodeDataCommandsDistributedTestBase.java
##########
@@ -206,51 +185,55 @@ public void testPrimitivesWithDataCommands() throws Exception {
Double doubleValue = Double.valueOf("111111.111111");
// Testing Byte Wrappers
- testGetPutLocateEntryFromShellAndGemfire(byteKey, byteValue, Byte.class, true, true);
+ testGetPutLocateEntryFromShellAndGeode(byteKey, byteValue, Byte.class, true, true);
// Testing Short Wrappers
- testGetPutLocateEntryFromShellAndGemfire(shortKey, shortValue, Short.class, true, true);
+ testGetPutLocateEntryFromShellAndGeode(shortKey, shortValue, Short.class, true, true);
// Testing Integer Wrappers
- testGetPutLocateEntryFromShellAndGemfire(integerKey, integerValue, Integer.class, true, true);
+ testGetPutLocateEntryFromShellAndGeode(integerKey, integerValue, Integer.class, true, true);
// Testing Float Wrappers
- testGetPutLocateEntryFromShellAndGemfire(floatKey, floatValue, Float.class, true, true);
+ testGetPutLocateEntryFromShellAndGeode(floatKey, floatValue, Float.class, true, true);
// Testing Double Wrappers
- testGetPutLocateEntryFromShellAndGemfire(doubleKey, doubleValue, Double.class, true, true);
+ testGetPutLocateEntryFromShellAndGeode(doubleKey, doubleValue, Double.class, true, true);
}
- private void testGetPutLocateEntryFromShellAndGemfire(final Serializable key,
- final Serializable value, Class klass, boolean addRegionPath, boolean expResult) {
+ private void testGetPutLocateEntryFromShellAndGeode(final Serializable key,
+ final Serializable value, Class<?> klass, boolean addRegionPath, boolean expResult) {
SerializableRunnableIF putTask = () -> {
Cache cache = ClusterStartupRule.getCache();
- Region region = cache.getRegion(DATA_REGION_NAME_PATH);
- assertNotNull(region);
+ Region<Object, Object> region =
+ Objects.requireNonNull(cache).getRegion(DATA_REGION_NAME_PATH);
Review comment:
This is another form of null assertion.
##########
File path: geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/GeodeDataCommandsDistributedTestBase.java
##########
@@ -94,47 +87,35 @@
protected MemberVM locator, server1, server2;
Review comment:
Declaring multiple fields or vars on one line is supposed to be disallowed by our code standard, but our current Spotless configuration disallows.
##########
File path: geode-dunit/src/main/java/org/apache/geode/management/internal/cli/commands/GeodeDataCommandsDistributedTestBase.java
##########
@@ -94,47 +87,35 @@
protected MemberVM locator, server1, server2;
- public void before() throws Exception {
- locator = cluster.startLocatorVM(0, locatorProperties());
- server1 = cluster.startServerVM(1, locator.getPort());
- server2 = cluster.startServerVM(2, locator.getPort());
- connectToLocator();
- }
-
- // extracted for convenience overriding in GemfireDataCommandsOverHttpDUnitTest to connect via
- // http
public void connectToLocator() throws Exception {
gfsh.connectAndVerify(locator);
}
private static void setupRegions(String regionName, String parRegionName) {
InternalCache cache = ClusterStartupRule.getCache();
- RegionFactory regionFactory =
+ assert cache != null;
Review comment:
[comment] I realize this is fairly subjective, so it's just a comment.
One best practice for tests is to never use Java `assert` and only use `assertThat` assertions.
Another best practice for tests is to never perform null checks and just let the test code throw NPE if something is null unless the main purpose of the test is to validate that something is not null (which would generally involve an assertion at or near the end of the test).
I think this and all of the following `assertThat(xxx).isNotNull()` lines are really just to make one optional inspection in IntelliJ happy. This inspection is recommended for Production code, but you can see the noise problem it causes for Test code.
There are a total of 14 of these lines in the develop version of this class and everyone of these is generally considered a bad practice in JUnit tests:
```
import static org.apache.geode.test.dunit.Assert.assertNotNull;
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataRegion);
assertNotNull(dataParRegion);
assertNotNull(dataParRegion);
assertNotNull(service.getMemberMXBean());
assertNotNull(service.getManagerMXBean());
assertNotNull(bean);
assertNotNull(bean);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
assertNotNull(region);
```
I recommend deleting all null assertions and following any these approaches instead:
1. In IntelliJ, go to Editor -> Inspections, search for `Constant conditions & exceptions` and change its scope to Product only (not Tests). Unfortunately we have test base classes like `GemfireDataCommandsDUnitTestBase` in the src/main which IntelliJ considers Product code so that doesn't work in this case.
2. Suppress this warning for this test class with `@SuppressWarnings("ConstantConditions")`
3. Do nothing and just let the code have warnings. We can't suppress or fix every single inspection in IntellIj. Some of the inspections should be universally applied blindly to all code (such as in this case), some of the inspections are only supposed to be used on the fly to search for certain things in the code base, some of them are actually binary opposites each asking you to do the opposite of the other.
Many of us have different IntelliJ Inspections enabled/disabled/customized. Unless we pick a standard Inspections configuration file to use for Geode like we do with Spotless, this is going to be messy and contentious.
In general, I support correcting all real warnings in both product and test code, but adding lots of null assertions for this Inspection probably makes the test code worse.
--
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] nabarunnag commented on pull request #6950: GEODE-9686: Remove deprecated elements from GemfireDataCommandsDistri…
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on pull request #6950:
URL: https://github.com/apache/geode/pull/6950#issuecomment-939561122
@kirklund thank you for these suggestions and I have implemented these in this and other PRs too.
--
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