8368001: java/text/Format/NumberFormat/NumberRoundTrip.java timed out #28443
Conversation
|
👋 Welcome back jlu! A progress list of the required criteria for merging this PR into |
|
@justin-curtis-lu This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 48 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
changed the title
|
@justin-curtis-lu The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| doTest(fmt, Long.valueOf(value)); | ||
| // Return a random value from -range..+range. | ||
| private static double randomDouble(double range) { | ||
| double a = Math.random(); |
There was a problem hiding this comment.
I would suggest using jdk.test.lib.RandomFactory, as this supports reproducible values in case of a randomised test failure.
import jdk.test.lib.RandomFactory;
...
* @library /test/lib
* @build jdk.test.lib.RandomFactory
...
private static final Random RANDOM = RandomFactory.getRandom();
...
private static double randomDouble(double range) {
double a = RANDOM.nextDouble();
...
There was a problem hiding this comment.
Thanks for the improvement, updated in dca988e.
naotoj
left a comment
naotoj
left a comment
There was a problem hiding this comment.
Good refactoring. One minor comment below
| * @bug 4266589 8031145 8164791 8316696 8368001 | ||
| * @summary NumberFormat round trip testing of parsing and formatting. | ||
| * This test checks 4 factory instances per locale against ~20 numeric inputs. | ||
| * Samples ~1/4 of the available locales from NumberFormat SPI. |
3 participants
JDK-8260555 exposed test/jdk/java/text/Format/NumberFormat/NumberRoundTrip.java as being susceptible to timeouts.
While the timeout factor is reverted, this test was testing too many inputs, and should be changed.
Originally, it was testing (~1000 locales * 4 factories) against ~30 numeric inputs. The test does not need to be exhaustive to this degree.
The total amount of locales is changed to sample around a 1/4 of those provided by the provider. The numeric inputs are reduced to about ~20. (The fixed ones remain, and the random ones are cut in half.)
Locally, I observe average execution for this test to be about ~1.3 seconds, compared to ~3.4 seconds previously. The PR also does a drive-by clean up of the test, i.e. getting rid of the unnecessary escaping + string manipulation/printing and separating the test inputs from the actual testing work done.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28443/head:pull/28443$ git checkout pull/28443Update a local copy of the PR:
$ git checkout pull/28443$ git pull https://git.openjdk.org/jdk.git pull/28443/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28443View PR using the GUI difftool:
$ git pr show -t 28443Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28443.diff
Using Webrev
Link to Webrev Comment