apache / apache/druid

Bring in the JUnit Assume feature to Test Case testPollOnDemand

Open
#12,436 2 comments 0 reactions 0 assignees View on GitHub
Area - Testing
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.