On Fri, 28 Apr 2023 12:25:19 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Not sure I quite understand what you are asking, so - >> >> 1. the test is good, it's a reasonable approach for a situation with a large >> space. An exhaustive tests using floats that I ran passes, no need (I >> think) to run it on every build. A similar test using doubles is not >> practical. So we are good here. >> >> 2. printing seed for tests involving Random. I think this should be a >> general rule to print the seed because if/when the test actually fails, we >> can reproduce with that particular seed. >> >> 3. resetting seed at the beginning of each test block. I am not sure that >> this is a good idea because we are cutting the amount of "randomness" to a >> third, **unless** this is intentional. Is it intentional? > > I'll clarify what I mean. The purpose of unit tests is to verify code works > correctly, by putting it through various scenarios and verifying the results > are what is expected. A test should be reliable, focused, fast and accurate. > > - Reliable: when run multiple times, it should either always fail or always > pass, the initial state of the machine should not affect the test outcome > - Focused: the goal is to test an isolated unit of code only; if it can't be > fully isolated, then assume that any dependencies work as specified > - Fast: test a reasonable subset of values and edge cases, exhaustive testing > is impossible and not a goal > - Accurate: the results should be verified to be correct against an > independent source (constants, test computed values, etc.) > > The existing test is poor in that regard. It runs random tests, which depend > on the initial state and timing of the machine, making it not reliable. It's > not accurate because it is verifying results with a call to the function > under test; if that function is broken then we'd be using a broken function > to verify results. > > So, to improve this test case we could: > - Make it reliable: use a fixed seed (use of `Random` with a fixed seed is > okay, it's specified exactly) > - Verify the results independently; currently the function could be written > as `return 0` and this test would pass I think the test is good, I just recommend removing the setSeed() in all but the first case. We cannot test the whole range, than would take too long. But we can put some effort to test a small subset of random values, which I think also serves a purpose. Don't forget that these tests are run by multiple agents many times over, to over time the number of tested values increases. To make the test repeatable, we print the seed. If it ever fails, we can reproduce the condition. I am not sure how you arrived at the last two points, but I think we are very close to finishing this PR. Perhaps @kevinrushforth could take a look? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1180614284
