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

8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods #4009

Closed
wants to merge 6 commits into from

Conversation

LanceAndersen
Copy link
Contributor

@LanceAndersen LanceAndersen commented May 13, 2021

HI all,

Please review the fix to HashesTest.java to support running on TestNG 7.4.

The fix adds a no-arg constructor which TestNG requires.

The change allows the test to run with the current jtreg as well as the upcoming jtreg

Best
Lance


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009
$ git checkout pull/4009

Update a local copy of the PR:
$ git checkout pull/4009
$ git pull https://git.openjdk.java.net/jdk pull/4009/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4009

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4009.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 13, 2021

👋 Welcome back lancea! 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
Copy link

@openjdk openjdk bot commented May 13, 2021

@LanceAndersen 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 label May 13, 2021
@openjdk openjdk bot added the rfr label May 13, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 13, 2021

Webrevs

srcDir=null;
lib= null;
builder=null;
}
Copy link
Contributor

@AlanBateman AlanBateman May 13, 2021

Choose a reason for hiding this comment

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

This looks like a workaround. Can you instead see if the fields can be changed to non-final and place the constructor with a method that has the @BeforeClass annotation?

Copy link
Contributor Author

@LanceAndersen LanceAndersen May 13, 2021

Choose a reason for hiding this comment

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

I can look to update the test. As I mentioned in my reply to Chris, I had thought about introducing an inner class bug decided to go the route of the default/no-arg constructor as it required fewer changes.

@ChrisHegarty
Copy link
Member

@ChrisHegarty ChrisHegarty commented May 13, 2021

The non-static state in this test class is initialized for each of the static testXXX scenarios. An alternative could be to move said state (four fields) into a static inner class, and have each of the testXXX scenarios create an instance of that class with the test-specific path. That would also allow the addition of the no-args public constructor to HashesTest, and the testXXX methods to be made non-static.

@LanceAndersen
Copy link
Contributor Author

@LanceAndersen LanceAndersen commented May 13, 2021

The non-static state in this test class is initialized for each of the static testXXX scenarios. An alternative could be to move said state (four fields) into a static inner class, and have each of the testXXX scenarios create an instance of that class with the test-specific path. That would also allow the addition of the no-args public constructor to HashesTest, and the testXXX methods to be made non-static.

I had originally thought about introducing an inner class but decided that given TestNG needs access to the default constructor, I chose that route vs. doing more of an update. I can revisit this though

@mlchung
Copy link
Member

@mlchung mlchung commented May 13, 2021

I also think it's good to fix this properly. Each test wants to run in an unique directory. Another solution is to make the fields non-final and add a new @BeforeMethod method to generate the unique pathname for each test. I think this seems a clean way (I sent you a patch to checkout).

@LanceAndersen
Copy link
Contributor Author

@LanceAndersen LanceAndersen commented May 13, 2021

The latest commit takes into account the feedback received

I also think it's good to fix this properly. Each test wants to run in an unique directory. Another solution is to make the fields non-final and add a new @BeforeMethod method to generate the unique pathname for each test. I think this seems a clean way (I sent you a patch to checkout).

The latest commit leverages @BeforeMethod vs an inner class and also includes some additional minor cleanup

Copy link
Member

@mlchung mlchung left a comment

Looks like the imports and whitespace are changed by IDE?? can you please revert these unintentional change.

Otherwise, the change looks good.

if (Files.exists(dest)) {
deleteDirectory(dest);
}
this.mods = dest.resolve("mods");
this.srcDir = dest.resolve("src");
Path srcDir = dest.resolve("src");
Copy link
Member

@mlchung mlchung May 13, 2021

Choose a reason for hiding this comment

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

you can just get rid of this local variable and do this in line 92:

     this.builder = new ModuleInfoMaker(dest.resolve("src"));

Copy link
Contributor Author

@LanceAndersen LanceAndersen May 13, 2021

Choose a reason for hiding this comment

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

Yes I can do that and realized that after I pushed the update

if (hashes != null) {
hashes.names().stream().sorted().forEach(n ->
System.out.format(" %s %s%n", n, toHex(hashes.hashFor(n)))
System.out.format(" %s %s%n", n, toHex(hashes.hashFor(n)))
Copy link
Member

@mlchung mlchung May 13, 2021

Choose a reason for hiding this comment

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

Nit: the extra whitespaces in line 384 and 387 may be added by IDE. Can you revert it.

Copy link
Contributor Author

@LanceAndersen LanceAndersen May 13, 2021

Choose a reason for hiding this comment

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

Intellij did that. I can tweak

Copy link
Contributor Author

@LanceAndersen LanceAndersen left a comment

I can look to revert the imports, that was an optimization via Intellij

if (hashes != null) {
hashes.names().stream().sorted().forEach(n ->
System.out.format(" %s %s%n", n, toHex(hashes.hashFor(n)))
System.out.format(" %s %s%n", n, toHex(hashes.hashFor(n)))
Copy link
Contributor Author

@LanceAndersen LanceAndersen May 13, 2021

Choose a reason for hiding this comment

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

Intellij did that. I can tweak

@openjdk
Copy link

@openjdk openjdk bot commented May 13, 2021

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

8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods

Reviewed-by: alanb, mchung

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

  • 88907bb: 8266904: Use function pointer typedefs in OopOopIterateDispatch
  • 301095c: 8266795: Remove dead code LowMemoryDetectorDisabler
  • 1e0ecd6: 8265605: Cannot call BootLoader::loadClassOrNull before initPhase2
  • 4086081: 8264846: Regression ~5% in J2dBench.bimg_misc on Linux after JDK-8263142
  • 2a2f105: 8267117: sun/hotspot/whitebox/CPUInfoTest.java fails on Ice Lake
  • 2667024: 8266881: Enable debug log for SSLEngineExplorerMatchedSNI.java
  • 6c107fd: 8264299: Create implementation of native accessibility peer for ScrollPane and ScrollBar Java Accessibility roles
  • 853ffdb: 8265934: Cleanup _suspend_flags and _special_runtime_exit_condition
  • f3c6cda: 8266162: Remove JPackage duplicate tests
  • a259ab4: 8258795: Update IANA Language Subtag Registry to Version 2021-05-11
  • ... and 35 more: https://git.openjdk.java.net/jdk/compare/dfe8833f5d9a9ac59857143a86d07f85769b8eae...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 label May 13, 2021
@@ -401,8 +386,7 @@ private ModuleHashes hashes(String name) {
private ModuleHashes hashes(ModuleFinder finder, String name) {
ModuleReference mref = finder.find(name).orElseThrow(RuntimeException::new);
try {
ModuleReader reader = mref.open();
try (InputStream in = reader.open("module-info.class").get()) {
try (ModuleReader reader = mref.open(); InputStream in = reader.open("module-info.class").get()) {
Copy link
Contributor

@AlanBateman AlanBateman May 14, 2021

Choose a reason for hiding this comment

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

Thats for doing the right thing and using BeforeMethod. The changes looks good.
There is some random re-formatting in hashes and deleteDirectory, I don't know if this was intended or not. I think we should at least resolve L389 before integrating.

Copy link
Contributor Author

@LanceAndersen LanceAndersen May 14, 2021

Choose a reason for hiding this comment

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

Thank you Alan. The re-formatting was done automagically by Intellij. I just pushed an update to address your suggestions.

@LanceAndersen
Copy link
Contributor Author

@LanceAndersen LanceAndersen commented May 14, 2021

/integrate

@openjdk openjdk bot closed this May 14, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2021

@LanceAndersen Since your change was applied there have been 50 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit e90388b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
4 participants