Attention is currently required from: Brian Norris, Peter Marheine, Thomas Heijligen.
Anastasia Klimchuk 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:
(2 comments)
Patchset:
PS9: Peter, this looks cool ! What do you think about sharing the link to the patch on the mailing list, perhaps some people would be interested to help testing. Even building on environment which is not-like-Jenkins would be really helpful.
File tests/udelay.c:
https://review.coreboot.org/c/flashrom/+/81545/comment/2a494375_efde4c23 : PS9, Line 41: This test could fail spuriously on a heavily-loaded system
Since this test is mostly meant to exercise the sleep-free case (as I explained in response to Anast […]
From my side, I would want to keep the test, with both lower and upper bound. I like more tests, not less! :) I really appreciate Peter adding a test in a patch!
Jenkins seems to be happy with the test, as we see. We can get incoming patches from anyone in the world, and unit tests are easy to run, and they give more confidence in the patch (for both sides).
In any case I agree to keep an eye on how the test is doing after it's merged, if it causes many flakes for upstream development we can remove upper bound. But I don't think there will be troubles.
ChromeOS fork could perhaps make a small one line diff, change the assert line, or disable the test (but first option is better, much less intrusive for downstreaming). It's not the only diff in unit tests. Peter you are closer to that, do you think it's fine?
-
Anastasia Klimchuk (Code Review)