[GitHub] [druid] Codegass opened a new issue, #12436: Bring in the JUnit Assume feature to Test Case testPollOnDemand

[GitBox <gi...@apache.org>]

Codegass opened a new issue, #12436:
URL: https://github.com/apache/druid/issues/12436

   ### Description & Motivation
   
   Hi,
   
   I would like to propose an idea of using Assume feature offered in JUnit 4&5 for some of the test cases in Druid. The Assume function is to replace Assertion in precondition checking. As [mentioned in the document](https://junit.org/junit4/javadoc/4.12/org/junit/Assume.html):
   
   > Assume functions is a set of methods useful for stating assumptions about the conditions in which a test is meaningful. A failed assumption does not mean the code is broken, but that the test provides no useful information.
   
   I notice that the test case [testPollOnDemand](https://github.com/apache/druid/blob/b86f2d4c2e935346d600e51b22403150ebd1501d/server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerTest.java#L187) can benefit from the Assume feature.
   
   Right now this case is asserting the precondition(`dataSourcesSnapshot`) before the actual testing target(`sqlSegmentsMetadataManager.forceOrWaitOngoingDatabasePoll()`) is executed.
   
   So test `testPollOnDemand` may fail due to 2 reasons:
     * The functions related to the testing precondition have bug (here is the `sqlSegmentsMetadataManager.getDataSourcesSnapshot()` `sqlSegmentsMetadataManager.useLatestSnapshotIfWithinDelay()`).
     * The functions related to Poll on Demand have bugs.(What we actually want to check with this test)
   
   Here I suggest replacing the `assertNull` function with the `assumeThat` function, so when the precondition fails, JUnit can skip it instead of report a failure. This can help us to focus on failure that is raise by the target testing function, and filter the precondition failure.
   
   #### Proposed Changes
   Before
   ```java
       DataSourcesSnapshot dataSourcesSnapshot = sqlSegmentsMetadataManager.getDataSourcesSnapshot();
       Assert.assertNull(dataSourcesSnapshot);
       // This should return false and not wait/poll anything as we did not schedule periodic poll
       Assert.assertFalse(sqlSegmentsMetadataManager.useLatestSnapshotIfWithinDelay());
       Assert.assertNull(dataSourcesSnapshot);
   ```
   
   After
   ```java
       DataSourcesSnapshot dataSourcesSnapshot = sqlSegmentsMetadataManager.getDataSourcesSnapshot();
       Assume.assumeThat(dataSourcesSnapshot, is(null));
       // This should return false and not wait/poll anything as we did not schedule periodic poll
       Assume.assumeFalse(sqlSegmentsMetadataManager.useLatestSnapshotIfWithinDelay());
       Assume.assumeThat(dataSourcesSnapshot, is(null));
   ```
   
   Please let me know if you think this proposal makes sense, I would be more than happy to try the refactoring. (If not, comments are appreciated.)


-- 
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: commits-unsubscribe@druid.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] FrankChen021 commented on issue #12436: Bring in the JUnit Assume feature to Test Case testPollOnDemand

[GitBox <gi...@apache.org>]

FrankChen021 commented on issue #12436:
URL: https://github.com/apache/druid/issues/12436#issuecomment-1101225323

   Thank you for your eyes on Druid and sugggestions.
   
   I don't know this JUnit feature well. Based on your description, do you mean that, if `getDataSourcesSnapshot` returns null, the statement
   
   ```java
    Assume.assumeThat(dataSourcesSnapshot, is(null));
   ```
   
   will force the current test to exit and mark it as something instead of failure?
   


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] Codegass commented on issue #12436: Bring in the JUnit Assume feature to Test Case testPollOnDemand

[GitBox <gi...@apache.org>]

Codegass commented on issue #12436:
URL: https://github.com/apache/druid/issues/12436#issuecomment-1119568016

   Thank you for the reply!
   
   JUnit Assume will let test skip if the condition is not satisfied.
   
   Here if `getDataSourcesSnapshot` returns null, then
   ```java
   Assume.assumeThat(dataSourcesSnapshot, is(null));
   ```
   will let the test continue.(`dataSourcesSnapshot` is null which matches the condition.)
   
   Only when `getDataSourcesSnapshot` not returns null, then
   ```java
   Assume.assumeThat(dataSourcesSnapshot, is(null));
   ```
   will just skip the test and leave an assume exception(which is weak than assert exception) in the test log like this:
   > org.junit.AssumptionViolatedException: got: \<va\l>, expected: is \<val\>.
   


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org