|
Ok. Looks good then.
Chris
On 5/8/19 8:26 PM, Jean Christophe
Beyler wrote:
Yes and actually the count variable too were
missing due to a mishap when generating the webrev... I had
this in my local diff:
public static void main(String[] args) {
int sizes[] = {1000, 10000, 100000};
+ double expected = 0;
+ int count = 0;
for (int currentSize : sizes) {
System.out.println("Testing size " +
currentSize);
Which fixes the issue you were seeing...
Thanks for the review!
Jc
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
--
--
--
|
