On Mon, 15 Jul 2024 09:04:24 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this test-only change which proposes to fix the >> test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? >> >> The JBS issue has comments which explains what causes the timeout. The >> commit in this PR addresses those issues by updating the test specific >> `ClassFileTransformer` to only instrument application specific class instead >> of all (core) classes. The test was introduced several years back to verify >> the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As >> such, the proposed changes in this PR continue to test that feature - we now >> merely use application specific class' native method to verify the semantics >> of that feature. >> >> Additional cleanups have been done in the test to make sure that if any >> exception does occur in the ClassFileTransformer then it does get recorded >> and that then causes the test to fail. >> >> With this change, I have run tier1 through tier6 and the test passes. >> Additionally, without this change I've run the test with a test repeat of >> 100 with virtual threads enabled and the test hangs occasionally (as >> expected). With this proposed fix, I have then run the test with virtual >> threads, around 300 times and it hasn't failed or hung in any of those >> instances. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > better formatting for transform() method of the test's agent Okay. Still seems to be a lot more changes than needed for the key update, but so be it. Thanks test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 50: > 48: // It assumes that a specific non-native library method will call a > specific > 49: // native method. The below may need to be updated based on library > changes. > 50: static String goldenNativeMethodName = "fooBarNativeMethod"; The comment block above seems no longer applicable now this is not a library class method. ------------- Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20154#pullrequestreview-2177321848 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677591632
