jujoramos commented on a change in pull request #4970:
URL: https://github.com/apache/geode/pull/4970#discussion_r412794182
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearWithExpirationDUnitTest.java
##########
@@ -0,0 +1,537 @@
+/*
Review comment:
> The test needs 30 minutes to finish all combination. And many tests
took more than 30 seconds each.
This is not a bad thing per se, I wanted to test all possible combinations
so it's expected for the distributed test to take some time to finish. I'll
remove some combinations, though, so the overall time will be reduced.
> Many combinations are unnecessary, for example we want to see the
expiration tasks are cancelled, we don't care what expiration. Only when we
want to see an expiration to be triggered, then we need some (not all)
expiration types. In my opinion, we can hard code to use DESTROY expiration
type only in this test.
We don't have to verify clear from accessor or server. Some other tests have
verified that. You can just use server to do clear.
Region types can also be reduced to a few. We have other tests to verify
that all the combination of region type can do clear successfully. We only need
to verify the expiration tasks are cleared in this test.
Having tests to verify several possible combinations is better than having
no tests at all, specially for the region types as we might change the
implementation class in the future... if we do, this test might be able to
catch possible regressions introduced, so I'd prefer to keep them all.
Regarding the `ExpirationAction`, I agree, will remove the `INVALIDATE` one and
just use `DESTROY`.
----------------------------------------------------------------
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:
us...@infra.apache.org
