[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: JDK-8306990: The guarantees given by Region's floor and ceiling functions should work for l
From:       Andy Goryachev <angorya () openjdk ! org>
Date:       2023-04-28 16:48:56
Message-ID: lqHEKu-Cs_j5Zv4BQk4phMoXMVnjArpxDWfIWHbxsdk=.8300a357-854a-4aaf-97e4-1161816b71c6 () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]