Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v4]
On Fri, 29 Nov 2024 15:04:20 GMT, Volkan Yazıcı wrote: >> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160) >> stresses that `Socket::connect()` failures should be handled such that the >> resultant state of the `Socket` and its underlying `SocketImpl` should >> match. In a similar fashion, `Socket::new` (which is using `bind()` and >> `connect()` under the hood) failures should not leave behind an open >> `SocketImpl` either. This change set, addressing >> [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest` >> verifying this behavior. > > Volkan Yazıcı has updated the pull request incrementally with one additional > commit since the last revision: > > Use record in `toString()` @vy Your change (at version fb75cbd1bed59283a5ce91430a4bfed96e2d803e) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/22439#issuecomment-2510893647
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v4]
On Fri, 29 Nov 2024 15:04:20 GMT, Volkan Yazıcı wrote: >> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160) >> stresses that `Socket::connect()` failures should be handled such that the >> resultant state of the `Socket` and its underlying `SocketImpl` should >> match. In a similar fashion, `Socket::new` (which is using `bind()` and >> `connect()` under the hood) failures should not leave behind an open >> `SocketImpl` either. This change set, addressing >> [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest` >> verifying this behavior. > > Volkan Yazıcı has updated the pull request incrementally with one additional > commit since the last revision: > > Use record in `toString()` Started a `tier1,2` run. Will integrate once it succeeds. - PR Comment: https://git.openjdk.org/jdk/pull/22439#issuecomment-2508045458
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v4]
On Fri, 29 Nov 2024 15:04:20 GMT, Volkan Yazıcı wrote: >> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160) >> stresses that `Socket::connect()` failures should be handled such that the >> resultant state of the `Socket` and its underlying `SocketImpl` should >> match. In a similar fashion, `Socket::new` (which is using `bind()` and >> `connect()` under the hood) failures should not leave behind an open >> `SocketImpl` either. This change set, addressing >> [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest` >> verifying this behavior. > > Volkan Yazıcı has updated the pull request incrementally with one additional > commit since the last revision: > > Use record in `toString()` LGTM - if the test is stable ;-) - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22439#pullrequestreview-2470137245
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v3]
On Fri, 29 Nov 2024 14:19:09 GMT, Daniel Fuchs wrote:
>> Volkan Yazıcı has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add `MockSocketImpl#toString()` to aid test output visuals
>
> test/jdk/java/net/Socket/CtorFailTest.java line 166:
>
>> 164: }
>> 165: valueByKey.put("closeInvocationCounter",
>> closeInvocationCounter.get());
>> 166: return MockSocketImpl.class.getSimpleName() + valueByKey;
>
> Have you considered using record instead of Map? You can create a local
> record class within a method...
>
>
> record MockSocket(Exception bindException, Exception
> connectException) { }
> return new MockSocket(bindException, connectException).toString();
>
>
> (or you could name the record "TestCase" or whatever...)
Switched to using records.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22439#discussion_r1863651543
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v4]
> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160) > stresses that `Socket::connect()` failures should be handled such that the > resultant state of the `Socket` and its underlying `SocketImpl` should match. > In a similar fashion, `Socket::new` (which is using `bind()` and `connect()` > under the hood) failures should not leave behind an open `SocketImpl` either. > This change set, addressing > [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest` > verifying this behavior. Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision: Use record in `toString()` - Changes: - all: https://git.openjdk.org/jdk/pull/22439/files - new: https://git.openjdk.org/jdk/pull/22439/files/cf86230d..fb75cbd1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22439&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22439&range=02-03 Stats: 11 lines in 1 file changed: 0 ins; 9 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/22439.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22439/head:pull/22439 PR: https://git.openjdk.org/jdk/pull/22439
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v3]
On Fri, 29 Nov 2024 13:24:17 GMT, Volkan Yazıcı wrote:
>> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160)
>> stresses that `Socket::connect()` failures should be handled such that the
>> resultant state of the `Socket` and its underlying `SocketImpl` should
>> match. In a similar fashion, `Socket::new` (which is using `bind()` and
>> `connect()` under the hood) failures should not leave behind an open
>> `SocketImpl` either. This change set, addressing
>> [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest`
>> verifying this behavior.
>
> Volkan Yazıcı has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add `MockSocketImpl#toString()` to aid test output visuals
test/jdk/java/net/Socket/CtorFailTest.java line 166:
> 164: }
> 165: valueByKey.put("closeInvocationCounter",
> closeInvocationCounter.get());
> 166: return MockSocketImpl.class.getSimpleName() + valueByKey;
Have you considered using record instead of Map? You can create a local record
class within a method...
record MockSocket(Exception bindException, Exception connectException)
{ }
return new MockSocket(bindException, connectException).toString();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22439#discussion_r1863590552
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v2]
On Fri, 29 Nov 2024 12:29:41 GMT, Daniel Fuchs wrote: >> Volkan Yazıcı has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Further simplify the test > > test/jdk/java/net/Socket/CtorFailTest.java line 211: > >> 209: throw new UnsupportedOperationException(); >> 210: } >> 211: > > IIRC Junit should print the test method arguments before running a test. I'd > suggest adding a toString here, so that when a test fails we know which > scenario is failing. That is indeed a (really) nice to have. Added in cf86230d44c1bbde7e67be86692fa36721ddd28c. - PR Review Comment: https://git.openjdk.org/jdk/pull/22439#discussion_r1863518965
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v3]
> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160) > stresses that `Socket::connect()` failures should be handled such that the > resultant state of the `Socket` and its underlying `SocketImpl` should match. > In a similar fashion, `Socket::new` (which is using `bind()` and `connect()` > under the hood) failures should not leave behind an open `SocketImpl` either. > This change set, addressing > [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest` > verifying this behavior. Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision: Add `MockSocketImpl#toString()` to aid test output visuals - Changes: - all: https://git.openjdk.org/jdk/pull/22439/files - new: https://git.openjdk.org/jdk/pull/22439/files/5e492b7b..cf86230d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22439&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22439&range=01-02 Stats: 15 lines in 1 file changed: 15 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/22439.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22439/head:pull/22439 PR: https://git.openjdk.org/jdk/pull/22439
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v2]
On Fri, 29 Nov 2024 09:31:26 GMT, Volkan Yazıcı wrote: >> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160) >> stresses that `Socket::connect()` failures should be handled such that the >> resultant state of the `Socket` and its underlying `SocketImpl` should >> match. In a similar fashion, `Socket::new` (which is using `bind()` and >> `connect()` under the hood) failures should not leave behind an open >> `SocketImpl` either. This change set, addressing >> [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest` >> verifying this behavior. > > Volkan Yazıcı has updated the pull request incrementally with one additional > commit since the last revision: > > Further simplify the test test/jdk/java/net/Socket/CtorFailTest.java line 211: > 209: throw new UnsupportedOperationException(); > 210: } > 211: IIRC Junit should print the test method arguments before running a test. I'd suggest adding a toString here, so that when a test fails we know which scenario is failing. - PR Review Comment: https://git.openjdk.org/jdk/pull/22439#discussion_r1863455861
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v2]
> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160) > stresses that `Socket::connect()` failures should be handled such that the > resultant state of the `Socket` and its underlying `SocketImpl` should match. > In a similar fashion, `Socket::new` (which is using `bind()` and `connect()` > under the hood) failures should not leave behind an open `SocketImpl` either. > This change set, addressing > [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest` > verifying this behavior. Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision: Further simplify the test - Changes: - all: https://git.openjdk.org/jdk/pull/22439/files - new: https://git.openjdk.org/jdk/pull/22439/files/c16b748e..5e492b7b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22439&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22439&range=00-01 Stats: 19 lines in 1 file changed: 4 ins; 5 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/22439.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22439/head:pull/22439 PR: https://git.openjdk.org/jdk/pull/22439
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure [v2]
On Thu, 28 Nov 2024 17:01:32 GMT, Daniel Fuchs wrote:
>> Volkan Yazıcı has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Further simplify the test
>
> test/jdk/java/net/Socket/CtorFailTest.java line 108:
>
>> 106:
>> 107: static List testCases() {
>> 108: String exceptionMessage = "intentional test failure";
>
> It would be good to put a comment here to remind the reader what the first
> argument is and what the second argument is. Something like:
>
>
> // Arguments: Exception bindException, Exception connectException
Switched from using (untyped) `Arguments` to `MockSocketImpl` in
5e492b7bb83c6668f31d3e72aa6ce458221ee846.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22439#discussion_r1863220981
Re: RFR: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure
On Thu, 28 Nov 2024 14:30:02 GMT, Volkan Yazıcı wrote:
> [8343791](https://bugs.openjdk.org/browse/JDK-8343791) (addressed by #22160)
> stresses that `Socket::connect()` failures should be handled such that the
> resultant state of the `Socket` and its underlying `SocketImpl` should match.
> In a similar fashion, `Socket::new` (which is using `bind()` and `connect()`
> under the hood) failures should not leave behind an open `SocketImpl` either.
> This change set, addressing
> [8345176](https://bugs.openjdk.org/browse/JDK-8345176), adds `CtorFailTest`
> verifying this behavior.
test/jdk/java/net/Socket/CtorFailTest.java line 108:
> 106:
> 107: static List testCases() {
> 108: String exceptionMessage = "intentional test failure";
It would be good to put a comment here to remind the reader what the first
argument is and what the second argument is. Something like:
// Arguments: Exception bindException, Exception connectException
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22439#discussion_r1862502838
