On Mon, 15 Jul 2024 07:00:12 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - David's suggestion - remove cosmetic/style changes >> - no need to timeout=240 >> - include stdio.h in test native library >> - merge latest from master branch >> - 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out > > test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 44: > >> 42: * @comment The test uses asmlib/Instrumentor.java which relies on >> ClassFile API PreviewFeature. >> 43: * @run main/native/timeout=240 NativeMethodPrefixApp roleDriver >> 44: * @comment The test uses a higher timeout to prevent test timeouts >> noted in JDK-6528548 > > Is /timeout=240 (and the comment) needed now. If I read the old issue > correctly it was due to use a JDK mounted on a network file system. In https://github.com/openjdk/jdk/pull/19495#discussion_r1625470816 we had considered removing the higher timeout from this test. But then decided to let it stay at that time. I think we don't need it anymore, not even when the test runs with `-Xcomp`. The recent runs in our CI for this test haven't shown the necessity of using this higher timeout. I have updated the PR to remove this. I plan to run additional tests in our CI to be sure that the removal of the timeout doesn't cause unexpected failures. > test/jdk/java/lang/instrument/libNativeMethodPrefix.c line 24: > >> 22: */ >> 23: >> 24: #include "jni.h" > > I assume this needs `#include <stdio.h>`. Updated the PR to include this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677457539 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677454337
