8266254: Update to use jtreg 6 #1241

gdams · 2022-07-19T09:19:22Z

Backport required in order to bump to jtreg 6

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

Issues

JDK-8266254: Update to use jtreg 6
JDK-8265020: tests must be updated for new TestNG module name

Reviewers

Christoph Langer (@RealCLanger - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev pull/1241/head:pull/1241
$ git checkout pull/1241

Update a local copy of the PR:
$ git checkout pull/1241
$ git pull https://git.openjdk.org/jdk11u-dev pull/1241/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1241

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/1241.diff

bridgekeeper · 2022-07-19T09:20:32Z

👋 Welcome back gdams! 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 · 2022-07-19T09:21:16Z

This backport pull request has now been updated with issues from the original commit.

mlbridge · 2022-07-19T09:25:30Z

Webrevs

00: Full (3d19dec8)

gdams · 2022-07-19T18:01:41Z

The failing windows CI tests will pass when we actually bump the jtreg version in #1239

RealCLanger · 2022-07-20T09:49:32Z

@shipilev @ArnoZeller @GoeLin: What do you think about bumping jtreg in 11u? Good idea or not?

shipilev · 2022-07-20T09:57:53Z

@shipilev @ArnoZeller @GoeLin: What do you think about bumping jtreg in 11u? Good idea or not?

I think we need to run the entirety of tier1..4 to verify all the tests in 11u work fine with new jtreg. The jtreg bump frequently comes with required test updates, and not all of them are linked to jtreg update bug, unfortunately.

GoeLin · 2022-07-20T10:20:39Z

I think at some point we will need this update. Be as said above we should be careful. I'll run it through our testing.

ArnoZeller · 2022-07-21T07:22:23Z

I think we will also need:

https://bugs.openjdk.org/browse/JDK-8266461 - because of the newer testng.
https://bugs.openjdk.org/browse/JDK-8266250 - at least in our test run these two tests showed a failure in the improved version
https://bugs.openjdk.org/browse/JDK-8266460 - needed for jtreg 6

gdams · 2022-07-21T07:29:54Z

@ArnoZeller thanks for your feedback. I’ll open backports for those 3 commits.

GoeLin · 2022-07-21T07:35:25Z

Hi,
our testing shows a row of problems with the change. Arno already spotted some of the fixes in head, thanks! In addition there is
test/nashorn/src/jdk/nashorn/api/scripting/test/ScriptObjectMirrorTest.java:285: error: reference to assertEquals is ambiguous
assertEquals(42.0, obj.to(Double.class));

gdams · 2022-07-21T08:12:32Z

@ArnoZeller I've raised the 3 backports as mentioned:

8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods #1252
8266250: WebSocketTest and WebSocketProxyTest call assertEquals(List<byte[]>, List<byte[]>) #1253
8266460: java.io tests fail on null stream with upgraded jtreg/TestNG #1254

@GoeLin I'll look into that test failure, annoyingly the file was removed in JDK14u so will need some investigation.

gdams · 2022-07-21T12:57:02Z

I've raised https://bugs.openjdk.org/browse/JDK-8290813 to cover the additional nashorn test fix

gdams · 2022-07-21T14:24:27Z

nashorn test fix PR is here #1255. I've tested it locally and it passes on my machine.

GoeLin · 2022-07-22T09:32:04Z

Hi, we still see the WebSocket tests failing. Did you test the patch with jtreg 6 in place?
Things like this:
test/jdk/java/net/httpclient/websocket/WebSocketProxyTest.java:155: error: ';' expected
public boolean equals(Object o) {

gdams · 2022-07-22T10:22:49Z

Hi, we still see the WebSocket tests failing. Did you test the patch with jtreg 6 in place?
Things like this:
test/jdk/java/net/httpclient/websocket/WebSocketProxyTest.java:155: error: ';' expected
public boolean equals(Object o) {

I did test the patch but assumed that I was running the tests incorrectly when I saw that. I also noted that the same failure occurs in jdk17u?

ArnoZeller · 2022-07-22T11:24:33Z

Hi, we still see the WebSocket tests failing. Did you test the patch with jtreg 6 in place? Things like this: test/jdk/java/net/httpclient/websocket/WebSocketProxyTest.java:155: error: ';' expected public boolean equals(Object o) {

Hi GoeLin,
line 155 says:

record bytes(byte[] bytes) {
    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o instanceof bytes other) {
            return Arrays.equals(bytes(), other.bytes());
        }
        return false;
    }
    @Override
    public int hashCode() { return Arrays.hashCode(bytes()); }
    public String toString() {
        return "0x" + HexFormat.of()
                .withUpperCase()
                .formatHex(bytes());
    }
}

record was added with JDK14 I guess. We might have to adopt this to 11.

gdams · 2022-07-22T12:22:49Z

record was added with JDK14 I guess. We might have to adopt this to 11.

@ArnoZeller how many patches would be involved in this process?

ArnoZeller · 2022-07-22T12:53:16Z

record was added with JDK14 I guess. We might have to adopt this to 11.

@ArnoZeller how many patches would be involved in this process?

@gdams I do not mean to downport record - that would break the java spec. You just have to change the test coding to not use record.
I see that the backport is already pushed? Then the test has to be fixed, because this coding does not compile with JDK11.

gdams · 2022-07-22T12:54:29Z

@gdams I do not mean to downport record - that would break the java spec. You just have to change the test coding to not use record.
I see that the backport is already pushed? Then the test has to be fixed, because this coding does not compile with JDK11.

Understood - I'll see what I can do

gdams · 2022-07-22T14:24:20Z

@GoeLin I've got a fix for the WebSocket test failures. Take a look at #1265

RealCLanger

Looks good now. SAP nightlies show no regressions together with the other fixes.

openjdk · 2022-07-25T06:54:05Z

@gdams This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8266254: Update to use jtreg 6
8265020: tests must be updated for new TestNG module name

Reviewed-by: clanger

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

5d20cb6: 8210960: Allow --with-boot-jdk-jvmargs to work during configure
f00776b: 8285728: Alpine Linux build fails with busybox tar
a625058: 8239379: ProblemList serviceability/sa/sadebugd/DebugdConnectTest.java on OSX
9de7c4c: 8282860: Write a regression test for JDK-4164779
0174134: 8284367: JQuery UI upgrade from 1.12.1 to 1.13.1
37ca256: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java
a1a0459: 8290886: [11u]: Backport of JDK-8266250 introduced test failures
69a3003: 8289853: Update HarfBuzz to 4.4.1
c291490: 8283087: Create a test or JDK-4715503
f53f3e6: 8282548: Create a regression test for JDK-4330998
... and 42 more: https://git.openjdk.org/jdk11u-dev/compare/5e1ce54d6a11eac153a6e6487bc0b4ed89741b5b...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RealCLanger) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

GoeLin · 2022-07-25T10:01:21Z

Hi @gdams,
The SAP testing is green now with this change.
We run all the tests excluding a small set of shaky tests, but also excluding the headful tests.
This is quite a bunch of tests. Can you somehow make sure these work with jtreg 6?

gdams · 2022-07-25T10:14:09Z

@GoeLin I'll run the headful tests now to confirm that there are no broken ones.

gdams · 2022-07-25T13:45:47Z

@GoeLin I've run a full test run and saw one test failure on macos which also fails with jtreg5 so is unrelated

GoeLin · 2022-07-28T08:40:49Z

I'm a bit concerned about the windows tests. You say "The failing windows CI tests will pass when we actually bump the jtreg version in #1239". If this is the case, only integrate this change if 1239 is ready for integration, too. You may not break the pre-submit tests for others by this.

gdams · 2022-07-28T08:44:43Z

I'm a bit concerned about the windows tests. You say "The failing windows CI tests will pass when we actually bump the jtreg version in #1239". If this is the case, only integrate this change if 1239 is ready for integration, too. You may not break the pre-submit tests for others by this.

@GoeLin #1239 is ready for integration. I've switched it from draft and set it up as a dependant PR so it should be good to go now.

gdams · 2022-07-28T11:28:46Z

/integrate

openjdk · 2022-07-28T11:29:56Z

@gdams
Your change (at version 3d19dec) is now ready to be sponsored by a Committer.

gdams · 2022-07-28T11:58:36Z

@GoeLin could you sponsor please?

RealCLanger · 2022-07-28T12:23:00Z

/sponsor

openjdk · 2022-07-28T12:24:11Z

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

c61acb5: 8258946: Fix optimization-unstable code involving signed integer overflow
5d20cb6: 8210960: Allow --with-boot-jdk-jvmargs to work during configure
f00776b: 8285728: Alpine Linux build fails with busybox tar
a625058: 8239379: ProblemList serviceability/sa/sadebugd/DebugdConnectTest.java on OSX
9de7c4c: 8282860: Write a regression test for JDK-4164779
0174134: 8284367: JQuery UI upgrade from 1.12.1 to 1.13.1
37ca256: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java
a1a0459: 8290886: [11u]: Backport of JDK-8266250 introduced test failures
69a3003: 8289853: Update HarfBuzz to 4.4.1
c291490: 8283087: Create a test or JDK-4715503
... and 43 more: https://git.openjdk.org/jdk11u-dev/compare/5e1ce54d6a11eac153a6e6487bc0b4ed89741b5b...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-28T12:24:30Z

@RealCLanger @gdams Pushed as commit 00d9483.

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

Backport 3482cb8

3d19dec

openjdk bot changed the title ~~Backport 3482cb87fd94e6445ca243722851daa90c00a2c8~~ 8266254: Update to use jtreg 6 Jul 19, 2022

openjdk bot added backport rfr Pull request is ready for review labels Jul 19, 2022

RealCLanger approved these changes Jul 25, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 25, 2022

openjdk bot mentioned this pull request Jul 28, 2022

8268185: Update GitHub Actions for jtreg 6 #1239

Closed

3 tasks

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 28, 2022

openjdk bot added the integrated Pull request has been integrated label Jul 28, 2022

openjdk bot closed this Jul 28, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 28, 2022

gdams deleted the jtreg6_ng branch July 28, 2022 12:24

openjdk bot removed the sponsor Pull request is ready to be sponsored label Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8266254: Update to use jtreg 6 #1241

8266254: Update to use jtreg 6 #1241

gdams commented Jul 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 19, 2022

openjdk bot commented Jul 19, 2022

mlbridge bot commented Jul 19, 2022

gdams commented Jul 19, 2022

RealCLanger commented Jul 20, 2022 •

edited

Loading

shipilev commented Jul 20, 2022

GoeLin commented Jul 20, 2022

ArnoZeller commented Jul 21, 2022

gdams commented Jul 21, 2022

GoeLin commented Jul 21, 2022 •

edited

Loading

gdams commented Jul 21, 2022

gdams commented Jul 21, 2022

gdams commented Jul 21, 2022

GoeLin commented Jul 22, 2022

gdams commented Jul 22, 2022 •

edited

Loading

ArnoZeller commented Jul 22, 2022

gdams commented Jul 22, 2022

ArnoZeller commented Jul 22, 2022 •

edited

Loading

gdams commented Jul 22, 2022

gdams commented Jul 22, 2022 •

edited

Loading

RealCLanger left a comment

openjdk bot commented Jul 25, 2022 •

edited

Loading

GoeLin commented Jul 25, 2022

gdams commented Jul 25, 2022

gdams commented Jul 25, 2022

GoeLin commented Jul 28, 2022

gdams commented Jul 28, 2022

gdams commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

gdams commented Jul 28, 2022

RealCLanger commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

8266254: Update to use jtreg 6 #1241

8266254: Update to use jtreg 6 #1241

Conversation

gdams commented Jul 19, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jul 19, 2022

openjdk bot commented Jul 19, 2022

mlbridge bot commented Jul 19, 2022

Webrevs

gdams commented Jul 19, 2022

RealCLanger commented Jul 20, 2022 • edited Loading

shipilev commented Jul 20, 2022

GoeLin commented Jul 20, 2022

ArnoZeller commented Jul 21, 2022

gdams commented Jul 21, 2022

GoeLin commented Jul 21, 2022 • edited Loading

gdams commented Jul 21, 2022

gdams commented Jul 21, 2022

gdams commented Jul 21, 2022

GoeLin commented Jul 22, 2022

gdams commented Jul 22, 2022 • edited Loading

ArnoZeller commented Jul 22, 2022

gdams commented Jul 22, 2022

ArnoZeller commented Jul 22, 2022 • edited Loading

gdams commented Jul 22, 2022

gdams commented Jul 22, 2022 • edited Loading

RealCLanger left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 25, 2022 • edited Loading

GoeLin commented Jul 25, 2022

gdams commented Jul 25, 2022

gdams commented Jul 25, 2022

GoeLin commented Jul 28, 2022

gdams commented Jul 28, 2022

gdams commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

gdams commented Jul 28, 2022

RealCLanger commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

openjdk bot commented Jul 28, 2022

gdams commented Jul 19, 2022 •

edited by openjdk bot

Loading

RealCLanger commented Jul 20, 2022 •

edited

Loading

GoeLin commented Jul 21, 2022 •

edited

Loading

gdams commented Jul 22, 2022 •

edited

Loading

ArnoZeller commented Jul 22, 2022 •

edited

Loading

gdams commented Jul 22, 2022 •

edited

Loading

openjdk bot commented Jul 25, 2022 •

edited

Loading