Re: RFR: 8281243: Test java/lang/instrument/RetransformWithMethodParametersTest.java is failing

2022-02-07 Thread Alex Menkov
On Fri, 4 Feb 2022 18:17:33 GMT, Daniel D. Daugherty  wrote:

> Thumbs up on the changes.
> 
> However, there's no details on how this fix was tested. Please clarify. So 
> far this test failure has been seen intermittently in Tier3.

Failure reports clearly show that the root cause is method sorting (ctor and 
method1 are in different order in original and reconstructed bytes), so I 
manually verified that now target class has the only method.
Also I run the test with --test-repeat 100 on supported platforms.

-

PR: https://git.openjdk.java.net/jdk/pull/7345


Re: RFR: 8281243: Test java/lang/instrument/RetransformWithMethodParametersTest.java is failing

2022-02-04 Thread Leonid Mesnik
On Fri, 4 Feb 2022 11:18:39 GMT, Alex Menkov  wrote:

> The test expects ClassFileReconstituter restores exactly the same bytes as 
> original classbytes.
> This can be wrong if the class has more than 1 method (due to method sorting 
> in the VM).
> MethodParametersTarget class had only 1 method (method1), but didn't have 
> constructors. This caused declaration of implicit default constructor, so the 
> class actually had 2 methods.
> The fix converts the method to constructor to avoid default constructor 
> declaration.

Marked as reviewed by lmesnik (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7345


Re: RFR: 8281243: Test java/lang/instrument/RetransformWithMethodParametersTest.java is failing

2022-02-04 Thread Daniel D . Daugherty
On Fri, 4 Feb 2022 11:18:39 GMT, Alex Menkov  wrote:

> The test expects ClassFileReconstituter restores exactly the same bytes as 
> original classbytes.
> This can be wrong if the class has more than 1 method (due to method sorting 
> in the VM).
> MethodParametersTarget class had only 1 method (method1), but didn't have 
> constructors. This caused declaration of implicit default constructor, so the 
> class actually had 2 methods.
> The fix converts the method to constructor to avoid default constructor 
> declaration.

Thumbs up on the changes.

However, there's no details on how this fix was tested. Please clarify.
So far this test failure has been seen intermittently in Tier3.

-

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7345


Re: RFR: 8281243: Test java/lang/instrument/RetransformWithMethodParametersTest.java is failing

2022-02-04 Thread Serguei Spitsyn
On Fri, 4 Feb 2022 11:18:39 GMT, Alex Menkov  wrote:

> The test expects ClassFileReconstituter restores exactly the same bytes as 
> original classbytes.
> This can be wrong if the class has more than 1 method (due to method sorting 
> in the VM).
> MethodParametersTarget class had only 1 method (method1), but didn't have 
> constructors. This caused declaration of implicit default constructor, so the 
> class actually had 2 methods.
> The fix converts the method to constructor to avoid default constructor 
> declaration.

Looks good to me.
Thanks,
Serguei

-

Marked as reviewed by sspitsyn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7345


RFR: 8281243: Test java/lang/instrument/RetransformWithMethodParametersTest.java is failing

2022-02-04 Thread Alex Menkov
The test expects ClassFileReconstituter restores exactly the same bytes as 
original classbytes.
This can be wrong if the class has more than 1 method (due to method sorting in 
the VM).
MethodParametersTarget class had only 1 method (method1), but didn't have 
constructors. This caused declaration of implicit default constructor, so the 
class actually had 2 methods.
The fix converts the method to constructor to avoid default constructor 
declaration.

-

Commit messages:
 - Fixed indent
 - Converted target class method to ctor

Changes: https://git.openjdk.java.net/jdk/pull/7345/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7345&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8281243
  Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7345.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7345/head:pull/7345

PR: https://git.openjdk.java.net/jdk/pull/7345