Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8077371: Binary files in JAXP test should be removed #13537

Closed
wants to merge 15 commits into from

Conversation

mahendrachhipa
Copy link
Member

@mahendrachhipa mahendrachhipa commented Apr 19, 2023

Test is updated to create the binary files during test execution.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8077371: Binary files in JAXP test should be removed (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13537/head:pull/13537
$ git checkout pull/13537

Update a local copy of the PR:
$ git checkout pull/13537
$ git pull https://git.openjdk.org/jdk.git pull/13537/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13537

View PR using the GUI difftool:
$ git pr show -t 13537

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13537.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 19, 2023

👋 Welcome back mchhipa! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 19, 2023
@openjdk
Copy link

openjdk bot commented Apr 19, 2023

@mahendrachhipa The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 19, 2023
@mlbridge
Copy link

mlbridge bot commented Apr 19, 2023

Copy link
Member

@JoeWang-Java JoeWang-Java left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall

test.createTestFile(JDK[ver - 6]);
}
if(!Files.exists(Path.of(filePath, jdkVersion+"t"+FILENAME_DURATION ))) {
DatatypeFactory dtf = DatatypeFactory.newInstance();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably share the factory rather than creating one in each loop and twice.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original version of this test was validating serialization compatibility with previous releases.

The current changes seems to have removed that validation in the re-write of the test

One option is to convert the various *.ser files to a byte array and have the test write them out or read them from a stream to do the validation

@dfuch
Copy link
Member

dfuch commented Apr 20, 2023

The original version of this test was validating serialization compatibility with previous releases.

The current changes seems to have removed that validation in the re-write of the test

One option is to convert the various *.ser files to a byte array and have the test write them out or read them from a stream to do the validation

Good point. You can also base64-encode the bytes into a string - here is an example:
https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/logging/HigherResolutionTimeStamps/SerializeLogRecord.java

@LanceAndersen
Copy link
Contributor

The original version of this test was validating serialization compatibility with previous releases.
The current changes seems to have removed that validation in the re-write of the test
One option is to convert the various *.ser files to a byte array and have the test write them out or read them from a stream to do the validation

Good point. You can also base64-encode the bytes into a string - here is an example: https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/logging/HigherResolutionTimeStamps/SerializeLogRecord.java

Trivial code to create a Byte array in addition to Daniel's example

    /**
     * Utility method which takes an byte array and converts to byte array
     * declaration.  For example:
     * <pre>
     *     {@code
     *        var fooJar = Files.readAllBytes(Path.of("foo.jar"));
     *        var result = createByteArray(fooJar, "FOOBYTES");
     *      }
     * </pre>
     * @param bytes A byte array used to create a byte array declaration
     * @param name Name to be used in the byte array declaration
     * @return The formatted byte array declaration
     */
    public static String createByteArray(byte[] bytes, String name) {
        StringBuilder sb = new StringBuilder(bytes.length * 5);
        Formatter fmt = new Formatter(sb);
        fmt.format("    public static byte[] %s = {", name);
        final int linelen = 8;
        for (int i = 0; i < bytes.length; i++) {
            if (i % linelen == 0) {
                fmt.format("%n        ");
            }
            fmt.format(" (byte) 0x%x,", bytes[i] & 0xff);
        }
        fmt.format("%n    };%n");
        return sb.toString();
    }

@mahendrachhipa
Copy link
Member Author

mahendrachhipa commented Apr 21, 2023

Thanks for your review comments. I will implement them and push again.

Copy link
Member

@JoeWang-Java JoeWang-Java left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok. Probably note in the relevant data classes the origin of the data (the .ser file generated with the JDK version).

@openjdk
Copy link

openjdk bot commented Apr 28, 2023

@mahendrachhipa 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:

8077371: Binary files in JAXP test should be removed

Reviewed-by: joehw

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 236 new commits pushed to the master branch:

  • defc7e0: 8318454: TestLayoutPaths broken on Big Endian platforms after JDK-8317837
  • 3c70f2c: 8318418: hsdis build fails with system binutils on Ubuntu
  • 15acf4b: 8318324: Drop redundant default methods from FFM API
  • 1a09835: 8317358: G1: Make TestMaxNewSize use createTestJvm
  • 47bb1a1: 8318415: Adjust describing comment of os_getChildren after 8315026
  • 80bd22d: 8316144: Remove unused field jdk.internal.util.xml.impl.XMLStreamWriterImpl.Element._Depth
  • c0e154c: 8318089: Class space not marked as such with NMT when CDS is off
  • 24bc5bd: 8318104: macOS 10.13 check in TabButtonAccessibility.m can be removed
  • e25a49a: 8318471: ProblemList compiler/sharedstubs/SharedTrampolineTest.java
  • ce8ebeb: 8317979: Use TZ database style abbreviations in the CLDR locale provider
  • ... and 226 more: https://git.openjdk.org/jdk/compare/49376e445210d5ebe3a99a4e647deecec51f0784...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 28, 2023
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed the original copyright year, e.g. 2014, 2023,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the copyright year and added the JDK version information in Data files.


// Generates the Java Pseudo code for base64 encoded string that can be cut & pasted into the test
final StringBuilder sb = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain the purpose of the following code in the setup method?. This appears to be what you used to generate the JDKXGregorianCalendarAndDurationSerData.java files. I don't see why this would be needed in a setup method.

Providing a method, along with comments of how to use it, to create byte arrays make sense, just not in a setup method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is added in setup method to create the serialized data with current JDK version (JDK under test). In addition to serialized data generated with old JDK version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I still disagree with this. Your comment in the test is as follows:

// Generates the Java Pseudo code for base64 encoded string that can be cut & pasted into the test

Which is what the method does and to facilitate it, as an example print it out so that it can be copied and pasted

System.out.println(sb);

Again, there is no reason to include this code in the setup method, it should be segmented out into its own method with instructions with how to create the encoded string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Lance, Initially I misunderstood the comment. Now I move the pseudo code generation code to separate methods.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay but lots of tasks are getting time sliced in this week :-)

I think you are getting closer. There is still some cleanup that can be done and more comments added for future maintainers.

As we look at creating or updating tests, we want to try to improve their readability and documentation.

Please see below for some examples of where we can look to improve the test

* Generates the Java Pseudo code for serialized Gregorian Calendar byte array that can be cut & pasted into the
* JDK<version>GregorianCalendarAndDurationSerData.java files.
* @param baos
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there needs to be a general comment describing how these methods are used to create the JDKGregorianCalendarAndDurationSerData.java files. There should also be a description/comment for the methods defined in GregorianCalendarAndDurationSerData.java

We need to try and put ourselves in the place of a future maintainer who needs to understand how to create a version of one of these files.

You could probably also create a method which generates a JDKGregorianCalendarAndDurationSerData.java file to save the developer from multiple cut an pastes.

At a minimum, there really should be a step by step guide

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How these methods can be used, added comment regarding this under setup method. Added description for the methods defined in GregorianCalendarAndDurationSerData.java file. Now developer should be able to create the GregorianCalendarAndDurationSerData.java file after reading these comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comment above in the setUp method as I feel we can do more here

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay, with the RDP1. fast approaching, it has been difficult getting back to this due to other priorities.

Please see the comments below as I feel we can do more to make the tests more streamline/maintainable.

Another datapoint to consider is that this might be a good time to convert the test from testng to junit as that is the preferred direction for our future tests.

// generatePseudoCodeForGregCalSerBytes(baos);
// generatePseudoCodeForGregCalSerBytesAsBase64(base64);
// generatePseudoCodeForDurationSerBytes(baos2);
// generatePseudoCodeForDurationSerBytesAsBase64(base64dur);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not like this being hidden in the setup method

I still would look to have a method that actually creates the classes. One example of this:

open/test/jdk/java/io/Serializable/failureAtomicity/FailureAtomicity.java and the createSrc method

The intent is to make this easier to update/enhance going forward. I would probably have. separate utility class/template which can be run to create the saved serialization classes.

SerializationTest would have an introductory comment outlining how to add additional serialized at a via the utility class.

}
@DataProvider(name = "GregorianCalendarData")
public Object [][] gregorianCalendarDataBytes() {
return new Object [][] {{System.getProperty("java.version"), gregorianCalendarAndDurationSerData[0], EXPECTED_CAL},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really only need to call System.getProperty("java.version") once in the program and then save off its return value

* Generates the Java Pseudo code for serialized Gregorian Calendar byte array that can be cut & pasted into the
* JDK<version>GregorianCalendarAndDurationSerData.java files.
* @param baos
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comment above in the setUp method as I feel we can do more here

final ByteArrayInputStream bais = new ByteArrayInputStream(gcsd.getGregorianCalendarByteArray());
final ObjectInputStream ois = new ObjectInputStream(bais);
final XMLGregorianCalendar xgc = (XMLGregorianCalendar) ois.readObject();
Assert.assertEquals(xgc.toString(), gregorianDate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you:

import static org.testng.Assert.*;. then you can simply call assertEquals

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 14, 2023

@mahendrachhipa This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 11, 2023

@mahendrachhipa This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Aug 11, 2023
@mahendrachhipa
Copy link
Member Author

/open

@openjdk openjdk bot reopened this Sep 29, 2023
@openjdk
Copy link

openjdk bot commented Sep 29, 2023

@mahendrachhipa This pull request is now open

1. Now test uses Junit5.
2. Created the seperate Utility class to generate the JDK version specific java src file.
@JoeWang-Java
Copy link
Member

Thanks for the update.

I'm not sure it's necessary to repeat tests with Base64 input. I think Daniel's comment was that the data can be optionally base64-encoded into a string, not that the tests needed to be duplicated.

The generatePseudoCodeFor... methods in the Util class can be consolidated too.

@LanceAndersen
Copy link
Contributor

Thanks for the update.

I'm not sure it's necessary to repeat tests with Base64 input. I think Daniel's comment was that the data can be optionally base64-encoded into a string, not that the tests needed to be duplicated.

The generatePseudoCodeFor... methods in the Util class can be consolidated too.

Yes it does look like there is duplication where the tests are run using a byte array which represents the serialized object and a second time using a Base64 representation. We do not need both as the intent of the original test is to validate that the serialized object can be read across the JDK versions

Addition of copyright section in autogenerated source file.
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 15, 2023
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 15, 2023
@RogerRiggs
Copy link
Contributor

Please re-generate the files for each old revisions using the new template (and compare).
The "mechanically generated" notation is not in the versions in the PR.

Revert back to Formatter class from HexFormat class, so that Any Java version serialization code
can be generated.
Added "Mechanically generated" notation to already generated classes with Old Java versions.
@mahendrachhipa
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 20, 2023

Going to push as commit 4010642.
Since your change was applied there have been 262 commits pushed to the master branch:

  • fe52917: 8318457: Use prefix-less prepend methods directly to reduce branches in String concat expressions
  • 71c99a0: 8318448: Link PopupMenu/PopupMenuLocation.java failure to JDK-8259913
  • 2c23391: 8318101: Additional test cases for CSSAttributeEqualityBug
  • deadb9c: 8304684: Memory leak in DirectivesParser::set_option_flag
  • a03767c: 8318049: C2: assert(!failure) failed: Missed optimization opportunity in PhaseIterGVN
  • 848ecc1: 8318538: Add a way to obtain a strided var handle from a layout
  • b07da3a: 8317819: Scope should reflect lifetime of underying resource (mainline)
  • 6f1d896: 8318510: Serial: Remove TenuredGeneration::block_size
  • 8f4ebd8: 8317920: JDWP-agent sends broken exception event with onthrow option
  • cd25d1a: 8318296: Move Space::initialize to ContiguousSpace
  • ... and 252 more: https://git.openjdk.org/jdk/compare/49376e445210d5ebe3a99a4e647deecec51f0784...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 20, 2023
@openjdk openjdk bot closed this Oct 20, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 20, 2023
@openjdk
Copy link

openjdk bot commented Oct 20, 2023

@mahendrachhipa Pushed as commit 4010642.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@RogerRiggs
Copy link
Contributor

@mahendrachhipa it is conventional and preferred to allow reviewers and people who have commented on a PR a day to review/re-review before integrating.

@LanceAndersen
Copy link
Contributor

@mahendrachhipa it is conventional and preferred to allow reviewers and people who have commented on a PR a day to review/re-review before integrating.

Just to follow on to Roger's comment. There have been many changes to this PR since Joe approved this on April 28th so we definitely should have given more time to allow for the reviewers to go through the latest updates and provide feedback and/or approve

@JoeWang-Java
Copy link
Member

Agree with Roger and Lance. My previous approval could not be counted towards all the changes that had happened afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
5 participants