You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2020/10/27 02:21:09 UTC

[GitHub] [shardingsphere-elasticjob] wwj-go opened a new pull request #1663: Fix OneOffJobBootstrapTest.assertExecute and JobSpringNamespaceWithTypeTest.jobScriptWithJobTypeTest assert failed in an environment of insufficient performance

wwj-go opened a new pull request #1663:
URL: https://github.com/apache/shardingsphere-elasticjob/pull/1663


   Fixes #1662.
   
   Fix OneOffJobBootstrapTest.assertExecute and JobSpringNamespaceWithTypeTest.jobScriptWithJobTypeTest assert failed in an environment of insufficient performance
   
   


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

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

[GitHub] [shardingsphere-elasticjob] wwj-go commented on a change in pull request #1663: Fix OneOffJobBootstrapTest.assertExecute and JobSpringNamespaceWithTypeTest.jobScriptWithJobTypeTest assert failed in an environment of insufficient performance

Posted by GitBox <gi...@apache.org>.
wwj-go commented on a change in pull request #1663:
URL: https://github.com/apache/shardingsphere-elasticjob/pull/1663#discussion_r512379975



##########
File path: elasticjob-lite/elasticjob-lite-spring/elasticjob-lite-spring-namespace/src/test/java/org/apache/shardingsphere/elasticjob/lite/spring/namespace/job/JobSpringNamespaceWithTypeTest.java
##########
@@ -35,14 +38,22 @@
     @Resource
     private CoordinatorRegistryCenter regCenter;
     
+    private Scheduler scheduler;
+
     @After
-    public void tearDown() {
-        JobRegistry.getInstance().shutdown(scriptJobName);
+    public void tearDown() throws SchedulerException {
+        while (!scheduler.getCurrentlyExecutingJobs().isEmpty()) {
+            BlockUtils.waitingShortTime();
+        }
+        JobRegistry.getInstance().getJobScheduleController(scriptJobName).shutdown();
     }
     
     @Test
-    public void jobScriptWithJobTypeTest() {
-        BlockUtils.sleep(1000L);
-        assertTrue(regCenter.isExisted("/" + scriptJobName + "/sharding"));

Review comment:
       This condition is not certain. If we wait for 1 second the `regCenter.isExisted("/" + scriptJobName + "/sharding")` will return true in most cases，but in some case it returns false.
   `   BlockUtils.sleep(1000L);` This line make our assert rely on operating system(OS)'s time and scheduler which we should avoid in unit test,  because the behavior of OS is something we cannot decide.




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

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

[GitHub] [shardingsphere-elasticjob] wwj-go commented on a change in pull request #1663: Fix OneOffJobBootstrapTest.assertExecute and JobSpringNamespaceWithTypeTest.jobScriptWithJobTypeTest assert failed in an environment of insufficient performance

Posted by GitBox <gi...@apache.org>.
wwj-go commented on a change in pull request #1663:
URL: https://github.com/apache/shardingsphere-elasticjob/pull/1663#discussion_r512383248



##########
File path: elasticjob-lite/elasticjob-lite-core/src/test/java/org/apache/shardingsphere/elasticjob/lite/api/bootstrap/impl/OneOffJobBootstrapTest.java
##########
@@ -105,9 +105,9 @@ private Scheduler getScheduler(final OneOffJobBootstrap oneOffJobBootstrap) {
     }
 
     @SneakyThrows
-    private void blockUtilFinish(final OneOffJobBootstrap oneOffJobBootstrap) {
+    private void blockUtilFinish(final OneOffJobBootstrap oneOffJobBootstrap, final AtomicInteger counter) {
         Scheduler scheduler = getScheduler(oneOffJobBootstrap);
-        while (!scheduler.isStarted() || !scheduler.getCurrentlyExecutingJobs().isEmpty()) {

Review comment:
       This case fails in some case because there is a little period between `scheduler.isStarted() and !scheduler.getCurrentlyExecutingJobs().isEmpty()` .
   For the execution of a scheduler, it will set the start flag true, then set the scheduler.getCurrentlyExecutingJobs() not empty. If a thread check the while condition between the two steps , the `scheduler.isStarted()` returns true and the `scheduler.getCurrentlyExecutingJobs().isEmpty()` also returns true, then it will get away  from the loop and do assert，which results in 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.

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

[GitHub] [shardingsphere-elasticjob] Technoboy- merged pull request #1663: Fix OneOffJobBootstrapTest.assertExecute and JobSpringNamespaceWithTypeTest.jobScriptWithJobTypeTest assert failed in an environment of insufficient performance

Posted by GitBox <gi...@apache.org>.
Technoboy- merged pull request #1663:
URL: https://github.com/apache/shardingsphere-elasticjob/pull/1663


   


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

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

[GitHub] [shardingsphere-elasticjob] codecov-io commented on pull request #1663: Fix OneOffJobBootstrapTest.assertExecute and JobSpringNamespaceWithTypeTest.jobScriptWithJobTypeTest assert failed in an environment of insufficient performance

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1663:
URL: https://github.com/apache/shardingsphere-elasticjob/pull/1663#issuecomment-716952066


   # [Codecov](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663?src=pr&el=h1) Report
   > Merging [#1663](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663?src=pr&el=desc) into [master](https://codecov.io/gh/apache/shardingsphere-elasticjob/commit/b053952301b214df8b7067cfe97d4ff7d07dca6f?el=desc) will **increase** coverage by `0.03%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663/graphs/tree.svg?width=650&height=150&src=pr&token=8ZMVc4Yo4Z)](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1663      +/-   ##
   ============================================
   + Coverage     85.99%   86.03%   +0.03%     
     Complexity      105      105              
   ============================================
     Files           251      251              
     Lines          5677     5677              
     Branches        875      875              
   ============================================
   + Hits           4882     4884       +2     
     Misses          480      480              
   + Partials        315      313       -2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...e/shardingsphere/elasticjob/infra/env/IpUtils.java](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663/diff?src=pr&el=tree#diff-ZWxhc3RpY2pvYi1pbmZyYS9lbGFzdGljam9iLWluZnJhLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZWxhc3RpY2pvYi9pbmZyYS9lbnYvSXBVdGlscy5qYXZh) | `64.61% <0.00%> (+1.53%)` | `0.00% <0.00%> (ø%)` | |
   | [...ticjob/lite/internal/snapshot/SnapshotService.java](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663/diff?src=pr&el=tree#diff-ZWxhc3RpY2pvYi1saXRlL2VsYXN0aWNqb2ItbGl0ZS1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9lbGFzdGljam9iL2xpdGUvaW50ZXJuYWwvc25hcHNob3QvU25hcHNob3RTZXJ2aWNlLmphdmE=) | `82.53% <0.00%> (+1.58%)` | `1.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663?src=pr&el=footer). Last update [b053952...14fbb30](https://codecov.io/gh/apache/shardingsphere-elasticjob/pull/1663?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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