RFR: JDK-8306990: The guarantees given by Region's floor and ceiling functions should work for larger values [v2]

[Andy Goryachev]

On Fri, 28 Apr 2023 12:25:19 GMT, John Hendrikx <jhendrikx at 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