Attention is currently required from: Anastasia Klimchuk, Peter Marheine, Thomas Heijligen.
Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email )
Change subject: udelay: only use OS time for delays, except on DOS ......................................................................
Patch Set 9:
(1 comment)
File tests/udelay.c:
https://review.coreboot.org/c/flashrom/+/81545/comment/e25e8253_31a03ad3 : PS9, Line 41: This test could fail spuriously on a heavily-loaded system
do you think it would be reasonable to maintain this assertion as-is provided we ensure the tested delay is always short enough to avoid sleeping?
Ah, sorry, thanks for the sleep vs delay callout. The delay loop likely doesn't have the same magnitude of an issue, although it theoretically is still present. I'm fine with "try and see" here.
Jenkins seems to be happy with the test, as we see.
Sure, once. But what's to say it doesn't fail 1/100 times? 1/1000? Do we guarantee what system load is run on the same Jenkins instance (I know nothing about Jenkins)? What if we have 10 such 1% flaky tests? Or 100? Eventually, nobody trusts the tests.
This is especially close to my heart at the moment, because I've recently been spending more than a week de-flaking another open source project's test suite, which were full of sleep-related magic and race conditions. The lesson that came out of that for me is that just about any race condition can and will be exposed. (And yes, this test has a race condition baked in.)
But like I said, I'm not extremely concerned here, unless we see it start flaking in practice.
I like more tests, not less! :)
Flaky tests can be worse, at scale. But maybe flashrom is isolated enough (and run in its own tiny test environment) that "scale" doesn't matter much.
ChromeOS fork could perhaps make a small one line diff [...]
Yeah, if it does indeed flake much, we'd likely disable or modify the test on the CrOS side.
Anyway, I'd consider my original comment resolved, but Anastasia has other comments in here so I'll leave that up to her when to resolve.
Thanks!
-
Brian Norris (Code Review)