-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ForkingTaskRunnerTest
#16323
Fix ForkingTaskRunnerTest
#16323
Conversation
@kfaraz don't you think there should be |
btw, those counters used in metrics reporting, so removing static will cause change in those metrics... i mean from test perspective static causing races, but if we looking at metric level and WorkerTaskCountStatsProvider I can understand why it's static since it's "Proides task / task count status at the level of individual worker nodes" |
if testing framework makes sure that each test runs one after another and not in parallel, other option would be to reset static variables in some visible for test method. |
@IgorBerman , I am not sure I follow. How would this be affected if we make the fields non-static? |
Yes, I wanted to do this but I decided to do it later as I need to look at all the exceptions and if it would be better to not throw an exception at all and simply return a The changes in this PR are more straightforward and just meant to get the build right without altering any behaviour. |
oh, ok, if it's singleton then it will work and no need in static. I missed this part |
@@ -494,7 +493,7 @@ int waitForTaskProcessToComplete(Task task, ProcessHolder processHolder, File lo | |||
+ task.getId() | |||
+ " must be an array of strings.") | |||
); | |||
Assert.assertEquals(1L, (long) forkingTaskRunner.getWorkerFailedTaskCount()); | |||
Assert.assertEquals(0L, (long) forkingTaskRunner.getWorkerFailedTaskCount()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are both failed and successful counts 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ideally the failed count should be 1, but it is currently not being tracked.
We can fix up the code later as mentioned here: #16323 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, @kfaraz. LGTM
Thanks for the review, @AmatyaAvadhanula , @IgorBerman ! |
ForkingTaskRunner
uses static fields to track task counts which are reported in metrics.This causes some tests like
ForkingTaskRunnerTest.testInvalidTaskContextJavaOptsArray()
to give false positives if the variables had been set by a preceding test.Changes:
ForkingTaskRunner