On Wed, 27 Nov 2024 10:45:48 GMT, Alan Bateman <al...@openjdk.org> wrote:
> 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(). @AlanBateman, thanks so much for the thorough review and valuable tips. In 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. > 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. Removed tests using mocks in dfb1bb919d81cd07de5da702a91eb3895a7af4e6. I will submit a JBS+PR for these. ------------- PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2504027028
