You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/04/05 06:20:16 UTC
[GitHub] [cloudstack] Codegass opened a new issue, #6203: Bring in the JUnit Assume feature to Test Case testPollOnDemand
Codegass opened a new issue, #6203:
URL: https://github.com/apache/cloudstack/issues/6203
<!--
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release and main branch are affected too.
Always add information AFTER of these HTML comments, but no need to delete the comments.
-->
##### ISSUE TYPE
<!-- Pick one below and delete the rest -->
* Enhancement Request
##### COMPONENT NAME
<!--
Categorize the issue, e.g. API, VR, VPN, UI, etc.
-->
~~~
TEST
~~~
##### CLOUDSTACK VERSION
<!--
New line separated list of affected versions, commit ID for issues on main branch.
-->
~~~
main
~~~
##### CONFIGURATION
<!--
Information about the configuration if relevant, e.g. basic network, advanced networking, etc. N/A otherwise
-->
N/A
##### OS / ENVIRONMENT
<!--
Information about the environment if relevant, N/A otherwise
-->
N/A
##### SUMMARY
<!-- Explain the problem/feature briefly -->
###### Description
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.
<!--
From my personal knowledge, Some developers may only be familiar with the testing target, not the target's dependency. it will be inefficient for the developer to debug the part they do not quite understand. They should spend most of their time debugging the issue related to the code they wrote. So the ideal way is to use the Assume functions skipping the tests that have dependency issues
-->
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.
<!--
This behavior will lead to two possible situations when the test fails:
-->
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.
<!--
As a test case, it should focus on the test target itself, not the preconditions. The preconditions should be checked by other tests and should not let the test fails. So here I suggest replacing the Assert functions from [lines 190 to 193](https://github.com/apache/druid/blob/b86f2d4c2e935346d600e51b22403150ebd1501d/server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerTest.java#L190-L193) with Assume functions.
-->
###### 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, If so I can submit a PR accordingly, If not, comments are appreciated.
-->
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.)
<!--
### Refactor
Please check [Commit](https://github.com/Codegass/druid/commit/2235ceee6b7ce25cfd79b398b3331694e24cfe6d)
-->
--
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@cloudstack.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland closed issue #6203: Bring in the JUnit Assume Feature to `CABackgroundTaskTest`
Posted by GitBox <gi...@apache.org>.
DaanHoogland closed issue #6203: Bring in the JUnit Assume Feature to `CABackgroundTaskTest`
URL: https://github.com/apache/cloudstack/issues/6203
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on issue #6203: Bring in the JUnit Assume Feature to Three Test Cases
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on issue #6203:
URL: https://github.com/apache/cloudstack/issues/6203#issuecomment-1217986966
@Codegass could you propose a PR, or discuss further on dev@ mailing list https://cloudstack.apache.org/mailing-lists.html
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Codegass commented on issue #6203: Bring in the JUnit Assume Feature to Three Test Cases
Posted by GitBox <gi...@apache.org>.
Codegass commented on issue #6203:
URL: https://github.com/apache/cloudstack/issues/6203#issuecomment-1220659952
> @Codegass could you propose a PR, or discuss further on dev@ mailing list https://cloudstack.apache.org/mailing-lists.html
Thank you for the reply! I will soon propose a PR this weekend.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org