Fix race condition in unit test #2108
Conversation
narrieta
requested review from
kevinclark19a,
larohra,
pgombar and
ZhidongPeng
as code owners
| @staticmethod | ||
| def _wait_for(predicate): | ||
| start_time = datetime.datetime.now() | ||
| while RunCommandTestCase._to_seconds(datetime.datetime.now() - start_time) < 30: |
There was a problem hiding this comment.
So this one test could take up to 30 seconds? Any reason for waiting that long?
Granted, this should only happen if the threads were unable to start for some reason, which is not a normal case.
There was a problem hiding this comment.
No, the test takes 0.05-0,0.08 s (my local machine). 30 s is the timeout if things go wrong
There was a problem hiding this comment.
Yes, I was wondering if that timeout could be shorter so we fail faster, but I realized this probably won't happen often.
There was a problem hiding this comment.
Oh, I think I misunderstood your point. 30 s is probably too generous. I am trying to avoid the test being noisy if, for example, one is running the unit test in a laptop while using a chat app that is taking most of the cpu.
There was a problem hiding this comment.
Sounds like a bad chat app 😄
There was a problem hiding this comment.
And probably we should add a verification to the entire suite to let us know if the run takes more than some given time. A bad mock, a regression in prod/test code may go unnoticed until somebody stumbles into a slow test run and realizes something is not correct.
There was a problem hiding this comment.
I can look into the Github Actions docs; I feel like this sort of thing should be easy to automate as a separate, not required job.
There was a race condition between the child process creating its file and the callback being invoked. Fixed by waiting for the callbacks explicitly.