On Wed, 27 Nov 2024 10:45:48 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Volkan Yazıcı has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Split tests into multiple files > > I scanned the test update. My personal view is that there way too much here. > JDK tests typically only use a small subset of the features that JUnit > provides. So very surprised to see the use of extensions in these tests. For > tests in this area then the priority should be on having tests that are easy > to maintain and diagnose when there are test failures, e.g. think about a > test failure in 2 years time where someone has to figure out test base with > different subclasses for different APIs. I can imagine someone being puzzled > by "Oio" in the name. My guess is that you mean "old I/O" but it looks really > odd here. > > Daniel may be able to give you other advice. My view is keep it simple, don't > over engineer the tests, and think about diagnosability when tests fail. In > this case I think we just need one test ConnectFailTest, or better name, that > exercises Socket.connect with the various scenarios. Think > CloseOnConnectFailureTestBase without the ceremony. The test methods can be > parametized so they execute with sockets created with new Socket() and > SocketChannel.open().socket(). > > I'm not sure what to say about the mock SocketImpl and testing the Socket > constructor, maybe you can create a test only issue in JBS and contribute it > that way. I'd prefer this PR to stay focused on Socket.connect. > @AlanBateman, thanks so much for the thorough review and valuable tips. In > [9454364](https://github.com/openjdk/jdk/commit/945436425fa953889448ebc4f8b7a80e1d4bd9e1), > replaced the existing tests with a single `ConnectFailTest` and tried to be > conservative on used JUnit features. Let me know if I captured the design you > outlined. ConnectTestFail could be changed to use parameterized tests with a method source than produces a Socket and SocketChannel.open().socket(). If you had that then it could eliminate the duplicate tests and would allow most or all of the functional interfaces to be removed. I think it good to find shorter names for many of the methods and variables as they are way too long in places, e.g. connectedSocketShouldNotBeClosedWhenConnectWithUnresolvedAddressFails. Did you mean to leave ThrowingSocketImpl in the PR, I thought that was for the other issue. Also are you expecting ServerSocketTestUtil to be used by other tests? ------------- PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2504283711
