Bring in the JUnit Assume feature to Test Case testPollOnDemand

[Codegass]

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:

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 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

    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

    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.)

[FrankChen021]

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

 Assume.assumeThat(dataSourcesSnapshot, is(null));

will force the current test to exit and mark it as something instead of failure?

[Codegass]

Thank you for the reply!

JUnit Assume will let test skip if the condition is not satisfied.

Here if getDataSourcesSnapshot returns null, then

Assume.assumeThat(dataSourcesSnapshot, is(null));

will let the test continue.(dataSourcesSnapshot is null which matches the condition.)

Only when getDataSourcesSnapshot not returns null, then

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: <val>, expected: is <val>.