Bring in the JUnit Assume feature to Test Case testPollOnDemand
- Dominant language
- Java
- Stars
- 14.1k
- Forks
- 3.8k
- Avg merge
- 2d 58m
- Merged PRs (30d)
- 233
Description
### 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.)
Contributor guide
Research direction
Start with server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerTest.java and the testPollOnDemand test. Review its current precondition assertions and the linked JUnit Assume documentation, then run the targeted test. Done means the precondition checks skip the test when assumptions fail while the poll-on-demand behavior remains asserted.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- testing
- Issue type
- Refactor
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100