|
Hi JC,
I don't see how this builds. You used to have:
78 double expected = maxIteration;
Now you have:
81 expected = maxIteration * count;
And I don't see a declaration of "expected" anywhere.
Other than that the changes look fine.
thanks,
Chris
On 5/8/19 3:53 PM, Jean Christophe Beyler wrote:
Hi all,
Due to this failing pretty heavily a test it seems; is
there any way I could get a second reviewer so I can hopefully
calm the test bug demons?
Hi Serguei,
Done & Done; I think 10 instead of 5 will be ok. It
only continues running if the sampling interval average
has not converged so it should only lengthen degenerate
cases.
Thanks for the review,
Jc
Hi
Jc,
A couple of minor comments.
37 private static final int maxCount = 5;
Constant names are better to start from a capital
letter.
Also, I'd suggest 10 instead of 5 to make it more
reliable.
But it depend on how much it potentially can make
the test to run slower.
94 // If we failed maxCount times, through the exception.
A typo: replace "through"
with "throw".
Otherwise, I'm Okay with this fix.
No need for another webrev.
Thanks,
Serguei
On 5/7/19 19:47, Jean Christophe Beyler wrote:
Hi all,
Could I get a review for this test fix:
Backstory is:
When I fixed https://bugs.openjdk.java.net/browse/JDK-8215113,
I fixed this failing test in the "actually make
the error calculation correct". But it turns out
the test itself failed 7 out of 10 times and
when we tested it, we were apparently "lucky".
This fix makes the test pass 5k times so I am
hopeful it will not show up again.
Extra notes:
- I fixed this by adding a loop that
says: if we have not converged yet, try
again at max 5 times; generally we don't
converge if the sampling intervals chosen at
the start just have pushed us far enough
from the average we expect that it takes
more samples to get us back on track
Thanks,
Jc
Ps: FWIW, I'm going to look at the
flakyness of the rest of the suite and see
if anything pops up to reduce the test bug
noise of this suite
--
--
|
