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

[John Hendrikx]

On Thu, 27 Apr 2023 19:43:14 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> What are we trying to achieve here I'm wondering? Because if it is to slowly exhaustively test more and more of the problem space during each build, then I really disagree with the methodology.
>> 
>> We're confident that this is the correct solution at this time.  The test is only there to prevent regressions to the old broken situation. 
>> 
>> If you argued that it's a poor test, I'd agree; it is a poor test, and has been since the beginning; I can trivially pass the test with a code change that it would not detect as faulty (returning a constant, using round, etc. would all pass this test).
>
> 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

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1180341978