alirezazamani commented on issue #624: Stabilize 5 unstable tests URL: https://github.com/apache/helix/pull/624#issuecomment-558390660 @dasahcc I did put some general information about the fixes in the PR description. Here are more detail: 1- TestWorkflowControllerDataProvider: In this test, the Tread.sleep(4000) has been used followed by multiple Assert statements to check values such as cache.getJobConfigMap().size() and cache.getWorkflowConfigMap().size(). However, Thread.sleep() is not desirable since there might be fast or slow controller threads that reach the expected values sooner/later than expected. Hence here it is preferable to used TestHelper.verify to check read these values and check them in a loop. 2- TestDrop: In this test Thread.sleep has been used followed by Assert which is eliminated in this PR and has been substituted with TestHelper.verify. 3- TestGetLastScheduledTaskExecInfo: This test were rely on Thread.sleep(). This has been changed to TestHelper.verify. In this test, one second delay has been used to make sure controller has enough time to schedule the tasks. However, this one second is not enough in most of the cases. Hence, in this PR, instead of Thread.sleep, it is better and more robust to use TestHelper.verify and count the number of scheduled task by checking their start time. If number of tasks with start time reaches the expected number of scheduled tasks, it means the test can continue to the next steps. 4- TestStopWorkflow: In this test Thread.sleep has been used to make sure all of the tasks are running. Instead of Thread.sleep, I used pollForJobState to make sure jobs are running and I also got all the tasks status in the context and checked if the tasks have reached the running state. Hence, we can be sure that the queue are in desired state after these checks. Also, previously, a global variable has been use among all the tasks to stop the tasks. Having a global variable to stop all the task might cause race condition between the tasks (concurrent get and set with different order). This has been changed to local variable.
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
