Re: RFR: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist [v2]
On Sun, 26 May 2024 07:24:16 GMT, SendaoYan wrote: >> Hi all, >> When there is no `/usr/bin/expect` in system, `throw new SkippedException` >> will not make the jvm exit in `@BeforeAll` junit stage, thus this will cause >> this testcase run failed. So I make change from `throw new SkippedException` >> to `Assumptions.abort` to avoid this issue. >> Only change the testcase, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with two additional > commits since the last revision: > > - delete "static final int JCK_STATUS_BASE = 95;" > >Signed-off-by: sendaoYan > - 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist > >Signed-off-by: sendaoYan Check the other methods on the Assumptions class, they are older than this. - PR Comment: https://git.openjdk.org/jdk/pull/19403#issuecomment-2133314171
Re: RFR: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist [v2]
On Sun, 26 May 2024 07:24:16 GMT, SendaoYan wrote: >> Hi all, >> When there is no `/usr/bin/expect` in system, `throw new SkippedException` >> will not make the jvm exit in `@BeforeAll` junit stage, thus this will cause >> this testcase run failed. So I make change from `throw new SkippedException` >> to `Assumptions.abort` to avoid this issue. >> Only change the testcase, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with two additional > commits since the last revision: > > - delete "static final int JCK_STATUS_BASE = 95;" > >Signed-off-by: sendaoYan > - 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist > >Signed-off-by: sendaoYan Thanks for fixing that. I can see that `Assumptions.abort` is a rather new addition: since 5.9. What was the correct way to abort a test in these circumstances prior to that method? - PR Comment: https://git.openjdk.org/jdk/pull/19403#issuecomment-2133269802
Re: RFR: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist [v2]
On Sun, 26 May 2024 07:24:16 GMT, SendaoYan wrote: >> Hi all, >> When there is no `/usr/bin/expect` in system, `throw new SkippedException` >> will not make the jvm exit in `@BeforeAll` junit stage, thus this will cause >> this testcase run failed. So I make change from `throw new SkippedException` >> to `Assumptions.abort` to avoid this issue. >> Only change the testcase, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with two additional > commits since the last revision: > > - delete "static final int JCK_STATUS_BASE = 95;" > >Signed-off-by: sendaoYan > - 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist > >Signed-off-by: sendaoYan > /sponsor Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/19403#issuecomment-2132757338
Re: RFR: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist [v2]
On Sun, 26 May 2024 07:40:38 GMT, Daniel Jeliński wrote: > LGTM. I assume you verified it does the right thing. Thanks for the review and approved. The change has been verified. - PR Comment: https://git.openjdk.org/jdk/pull/19403#issuecomment-2132120584
Re: RFR: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist [v2]
On Sun, 26 May 2024 07:24:16 GMT, SendaoYan wrote: >> Hi all, >> When there is no `/usr/bin/expect` in system, `throw new SkippedException` >> will not make the jvm exit in `@BeforeAll` junit stage, thus this will cause >> this testcase run failed. So I make change from `throw new SkippedException` >> to `Assumptions.abort` to avoid this issue. >> Only change the testcase, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with two additional > commits since the last revision: > > - delete "static final int JCK_STATUS_BASE = 95;" > >Signed-off-by: sendaoYan > - 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist > >Signed-off-by: sendaoYan LGTM. I assume you verified it does the right thing. - Marked as reviewed by djelinski (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19403#pullrequestreview-2079496098
Re: RFR: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist [v2]
On Sun, 26 May 2024 06:16:44 GMT, Alan Bateman wrote:
>> test/jdk/java/io/IO/IO.java line 64:
>>
>>> 62: expect = Paths.get("/usr/bin/expect"); // os-specific path
>>> 63: if (!Files.exists(expect) || !Files.isExecutable(expect)) {
>>> 64: System.out.println("jtreg.SkippedException: '" + expect
>>> + "' not found");
>>
>> SkippedException works with jtreg tests only. For jUnit you need to use
>> [Assumptions.abort](https://junit.org/junit5/docs/5.9.1/api/org.junit.jupiter.api/org/junit/jupiter/api/Assumptions.html#abort(java.lang.String))
>
>> SkippedException works with jtreg tests only. For jUnit you need to use
>> [Assumptions.abort](https://junit.org/junit5/docs/5.9.1/api/org.junit.jupiter.api/org/junit/jupiter/api/Assumptions.html#abort(java.lang.String))
>
> Yes, the Assumptions API should be used here. We use that in several JUnit
> tests that skip when tests when they can't run and you want it to fail the
> test.
Thanks for the review and suggest. The code has been updated according the
suggest. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19403#discussion_r1615075147
Re: RFR: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist [v2]
> Hi all, > When there is no `/usr/bin/expect` in system, `throw new SkippedException` > will not make the jvm exit in `@BeforeAll` junit stage, thus this will cause > this testcase run failed. So I make change from `throw new SkippedException` > to `System.exit` to avoid this issue. When use `System.exit`, I think we > should use `othervm` mode in jtreg. > Only change the testcase, the risk is low. > > Thanks. SendaoYan has updated the pull request incrementally with two additional commits since the last revision: - delete "static final int JCK_STATUS_BASE = 95;" Signed-off-by: sendaoYan - 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist Signed-off-by: sendaoYan - Changes: - all: https://git.openjdk.org/jdk/pull/19403/files - new: https://git.openjdk.org/jdk/pull/19403/files/90fa1e13..57b4dee5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19403&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19403&range=00-01 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19403.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19403/head:pull/19403 PR: https://git.openjdk.org/jdk/pull/19403
