On Wed, 20 Aug 2025 17:05:59 GMT, Leo Korinth <lkori...@openjdk.org> wrote:
>> This changes the timeout factor from 4 to 1. Most of the changes add >> timeouts to individual test cases so that I am able to run them with a >> timeout factor of 0.7 (some margin to the checked in factor of one) >> >> In addition to changing the timeout factor, I am also using a library call >> to parse the timeout factor from the Java properties (I cannot use the >> library function everywhere as jtreg does not allow me to add @library >> notations to non test case files). >> >> My approach has been to run all tests, and afterwards updating those that >> fail due to the timeout factor. The amount of updated test cases is huge, >> and my strategy has been to quadruple the timeout if I could not directly >> see that less was needed. In a few places, I have added a bit more timeout >> so that it will work with the 0.7 timeout factor. >> >> These fixes have been created when I have ploughed through test cases: >> JDK-8352719: Add an equals sign to the modules statement >> JDK-8352709: Remove bad timing annotations from WhileOpTest.java >> JDK-8352074: Test MemoryLeak.java seems not to test what it is supposed to >> test >> CODETOOLS-7903937: JTREG uses timeout factor on socket timeout but not on >> KEEPALIVE >> CODETOOLS-7903961: Make default timeout configurable >> >> After the review, I will update the copyrights. >> >> I have run testing tier1-8. The last time with a timeout factor of 1 instead >> of 0.7. >> >> I got 4 timing related faults: >> 1) runtime/Thread/TestThreadDumpMonitorContention.java >> This is probably: https://bugs.openjdk.org/browse/JDK-8361370 >> 2) sun/tools/jhsdb/BasicLauncherTest.java >> I am unsure about this one, it has not failed on my runs before, even >> with a timeout factor of 0.7, maybe I was unlucky. >> 3) gc/stress/TestReclaimStringsLeaksMemory.java >> I updated this to 480 seconds, I finish this fairly fast (~14s) on my not >> very fast computer, but the Macs that fail are old x86-based ones. >> 4) sun/security/ssl/X509KeyManager/CertChecking.java >> This is a new test that I got on last rebase. I have added a timeout of >> 480 to it. >> >> In addition to these four tests, I have another one >> "java/lang/ThreadLocal/MemoryLeak.java" that earlier failed with a timeout >> factor of 0.7 but did not fail in the last run. I will *not* update that >> test case, because the extra time spent is strange and should be looked at. >> I have created JDK-8352074 on that test case, and I will probably create >> another bug describing the timeout I got. >> >> From the review of the cancelled "8356171: ... > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > update testing.md, remove makefile link, fix bad text > Before the default of 120 was multiplied by the timeoutFactor of 4 to given > 480. Now the value 480 is multiplied by the timeoutFactor of 1 to give 480. I identified some cases that doesn't follow this. Unclear whether they are intentional. test/hotspot/jtreg/compiler/tiered/Level2RecompilationTest.java line 36: > 34: * @build jdk.test.whitebox.WhiteBox > 35: * @run driver jdk.test.lib.helpers.ClassFileInstaller > jdk.test.whitebox.WhiteBox > 36: * @run main/othervm/timeout=960 -Xbootclasspath/a:. > -XX:+TieredCompilation Why not `480` in this case? test/hotspot/jtreg/runtime/cds/appcds/LotsOfSyntheticClasses.java line 40: > 38: * @requires vm.cds > 39: * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds > 40: * @run driver/timeout=8000 LotsOfSyntheticClasses I was expecting `500 * 4 = 2000`, instead of `8000` here. test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendResume2/SuspendResume2.java line 31: > 29: * @compile SuspendResume2.java > 30: * @run driver jdk.test.lib.FileInstaller . . > 31: * @run main/othervm/native/timeout=700 Why `700` instead of `480` in this file? test/jdk/java/rmi/transport/dgcDeadLock/DGCDeadLock.java line 59: > 57: public class DGCDeadLock implements Runnable { > 58: final static public int HOLD_TARGET_TIME = 25000; > 59: public static final double TEST_FAIL_TIME = (HOLD_TARGET_TIME + > 30000) * Math.max(TestLibrary.getTimeoutFactor(), 4); Why `max(...)`? If it's for backwards compatibility, shouldn't it be `(HOLD_TARGET_TIME + 30000) * 4 * TestLibrary.getTimeoutFactor()`? test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 60: > 58: * @comment skip running this test on 32 bit VM > 59: * @requires vm.bits == "64" > 60: * @run testng/othervm/timeout=960 -Xmx2g WhiteBoxResizeTest Why not `480`? test/jdk/java/util/PluggableLocale/LocaleNameProviderTest.java line 34: > 32: * @build com.foobar.Utils > 33: * com.bar.* > 34: * @run junit/othervm/timeout=960 -Djava.locale.providers=CLDR,SPI > LocaleNameProviderTest Why not `480`? test/jdk/jdk/jfr/event/oldobject/TestObjectDescription.java line 49: > 47: * @library /test/lib /test/jdk > 48: * @modules jdk.jfr/jdk.jfr.internal.test > 49: * @run main/othervm/timeout=960 -XX:TLABSize=2k > jdk.jfr.event.oldobject.TestObjectDescription Why not `480`? test/jdk/tools/jpackage/share/InstallDirTest.java line 69: > 67: * @compile -Xlint:all -Werror InstallDirTest.java > 68: * @requires (jpackage.test.SQETest != null) > 69: * @run main/othervm/timeout=4000 -Xmx512m jdk.jpackage.test.Main Why more than `4x` in this file? test/langtools/jdk/jshell/HangingRemoteAgent.java line 38: > 36: class HangingRemoteAgent extends RemoteExecutionControl { > 37: > 38: private static final int TIMEOUT = (int)(2000 * > Double.parseDouble(System.getProperty("test.timeout.factor", "1.0"))); why not `Utils.TIMEOUT_FACTOR`? test/langtools/jdk/jshell/UITesting.java line 148: > 146: } > 147: > 148: private static final long TIMEOUT = (long) (60_000 * > Double.parseDouble(System.getProperty("test.timeout.factor", "1.0"))); Why not `Utils.TIMEOUT_FACTOR`? ------------- PR Review: https://git.openjdk.org/jdk/pull/26749#pullrequestreview-3144985957 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294077875 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294085201 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294089550 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294108202 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294110136 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294113670 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294116148 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294119800 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294128741 PR Review Comment: https://git.openjdk.org/jdk/pull/26749#discussion_r2294129243
