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 2022/02/09 22:36:03 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #7339: GEODE-10021: Clean up RemoteTransactionDUnitTest
DonalEvans commented on a change in pull request #7339:
URL: https://github.com/apache/geode/pull/7339#discussion_r803121633
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -510,16 +505,16 @@ void verifyAfterRollback(OP op) {
expectedCust = new Customer("customer1", "address1");
expectedOrder = new Order("order1");
expectedRef = new Customer("customer1", "address1");
- assertEquals(expectedCust, custRegion.getEntry(custId).getValue());
- assertEquals(expectedOrder, orderRegion.getEntry(orderId).getValue());
+ assertThat(expectedCust).isEqualTo(custRegion.getEntry(custId).getValue());
+ assertThat(expectedOrder).isEqualTo(orderRegion.getEntry(orderId).getValue());
getCache().getLogger().info("SWAP:verifyRollback:" + orderRegion);
getCache().getLogger().info("SWAP:verifyRollback:" + orderRegion.getEntry(orderId2));
- assertNull(getCache().getTXMgr().getTXState());
- assertNull("" + orderRegion.getEntry(orderId2), orderRegion.getEntry(orderId2));
- assertNull(orderRegion.getEntry(orderId3));
- assertNull(orderRegion.get(orderId2));
- assertNull(orderRegion.get(orderId3));
- assertEquals(expectedRef, refRegion.getEntry(custId).getValue());
+ assertThat(getCache().getTXMgr().getTXState()).isNull();
+ assertThat(orderRegion.getEntry(orderId2)).as("" + orderRegion.getEntry(orderId2));
Review comment:
There seems be a missing `isNull()` on the end here.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -3666,27 +3570,24 @@ public boolean getSuccess() {
@Override
public void afterCreate(EntryEvent event) {
- fail("create not expected");
+ throw new UnsupportedOperationException("create not expected");
}
@Override
public void afterUpdate(EntryEvent event) {
- if (!success) {
- System.out.println("WE WIN!");
- success = true;
- } else {
- fail("Should have only had one update");
- }
+ assertThat(success).as("Should have only had one update").isTrue();
Review comment:
To preserve the original behaviour, this should be `assertThat(success).as("Should have only had one update").isFalse();`
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -633,19 +629,19 @@ public void testPRTXGet() {
@Override
public Object call() {
TXManagerImpl mgr = getCache().getTxManager();
- assertTrue(mgr.isHostedTxInProgress(txId));
+ assertThat(mgr.isHostedTxInProgress(txId)).isTrue();
TXStateProxy tx = mgr.getHostedTXState(txId);
System.out.println("TXRS:" + tx.getRegions());
- assertEquals(3, tx.getRegions().size());// 2 buckets for the two puts we
+ assertThat(3).isEqualTo(tx.getRegions().size());// 2 buckets for the two puts we
Review comment:
Small nitpick, but this and a few other assertions in this class should be swapped around so that the expected and actual values are in the correct order, e.g.:
```
assertThat(tx.getRegions().size()).isEqualTo(3);
```
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -1255,10 +1247,12 @@ public Object call() {
Customer customer2 = new Customer("customer2", "address2");
Customer fakeCust = new Customer("foo2", "bar2");
try {
- cust.removeAll(Arrays.asList(custId0, custId4, custId1, custId2, custId3, custId20));
- fail("expected exception that was not thrown");
- } catch (TransactionDataNotColocatedException e) {
+ assertThatThrownBy(() -> cust
+ .removeAll(Arrays.asList(custId0, custId4, custId1, custId2, custId3, custId20)))
+ .isInstanceOf(TransactionDataNotColocatedException.class);
+ } catch (Throwable e) {
mgr.rollback();
+ throw e;
}
Review comment:
To preserve the original behaviour, this should be:
```
assertThatThrownBy(() -> cust
.removeAll(Arrays.asList(custId0, custId4, custId1, custId2, custId3, custId20)))
.isInstanceOf(TransactionDataNotColocatedException.class);
mgr.rollback();
```
with no try/catch.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -3636,9 +3542,7 @@ public Object call() {
Region cust = getCache().getRegion(D_REFERENCE);
OneUpdateCacheListener rat =
(OneUpdateCacheListener) cust.getAttributes().getCacheListener();
- if (!rat.getSuccess()) {
- fail("The OneUpdateCacheListener didnt get an update");
- }
+ assertThat(rat.getSuccess()).isTrue();
Review comment:
The failure description from the original test is lost here. It would be good to add it back with `as()`
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -3695,42 +3596,26 @@ public void afterInvalidate(EntryEvent event) {
private boolean oneCreate;
public void checkSuccess() {
- if (oneDestroy && oneCreate) {
- // chill
- } else {
- fail("Didn't get both events. oneDestroy=" + oneDestroy + " oneCreate=" + oneCreate);
- }
+ assertThat(oneDestroy && oneCreate).isTrue();
Review comment:
The failure message is lost with this change. It would be good to keep it using `as()`.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -1872,12 +1858,8 @@ public Object call() {
getCache().getTxManager().begin();
Region r = getCache().getRegion(CUSTOMER);
r.put(new CustId(8), new Customer("name8", "address8"));
- try {
- getCache().getTxManager().commit();
- fail("expected exception that was not thrown");
- } catch (Exception e) {
- assertThat("AssertionError").isEqualTo(e.getCause().getMessage());
- }
Review comment:
This block is very confusing. There's a `fail()` which gets triggered if we don't throw an exception on calling `commit()`, but then we catch an exception and assert that it's an AssertionError (which is what gets thrown by `fail()`) and then just carry on as long as it is. This means that the only time this block doesn't result in a test failure is when we *don't* hit an exception in `commit()` so that we hit the `fail()` call and then catch and correctly assert on the exception it throws. It's worth checking to make sure, but I think this should just be:
```
getCache().getTxManager().commit();
```
with no asserts or try/catch at all.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/RemoteTransactionDUnitTest.java
##########
@@ -1969,8 +1966,8 @@ public Object call() {
final Integer txOnDatastore1_1 = (Integer) datastore1.invoke(getNumberOfTXInProgress);
final Integer txOnDatastore2_2 = (Integer) datastore2.invoke(getNumberOfTXInProgress);
- assertEquals(0, txOnDatastore1_1.intValue());
- assertEquals(0, txOnDatastore2_2.intValue());
+ assertThat(0).isEqualTo(txOnDatastore1_1.intValue());
+ assertThat(0).isEqualTo(txOnDatastore2_2.intValue());
Review comment:
This can be simplified to:
```
assertThat(txOnDatastore1_1.intValue()).isZero();
assertThat(txOnDatastore2_2.intValue()).isZero();
```
--
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