Take my "personal preference" comments with a grain of salt.
This test may be part of a test template, so if some of my comments
(e.g. 340/412) are because of your test modification, you can ignore them.
CliHelloProcessing.java
-----------------------
My personal preference is to have the names written out for testing,
unless there is a real reason to not.
ClientHelloExtensionProcessing.java? You can immediately tell what the
test is doing by the name.
74: Would you please include a printout of what's actually being sent
here? That would help people parse it instead of having to run it and
then print out the debug information. This might also come in handy to
help folks understand what you tested against when other problems with
other extensions come up.
74: You might mention that this might happen when clients that don't
support PSK are used.
248: You might mention that we know about names, but didn't support it
(IIRC), and would fail if we tried to use it.
283: Would it be better just to create a single static SSLContext, and
then create SSLEngines from that, rather that creating a bunch of
SSLContexts? Same with the packet bufs.
336: Personal preference again: it could be just an Exception, rather
than RTE, since this is not a programming error.
340: Why are passing proto, then not using it?
347: If you're going to be running with debug on, maybe include the
packet dump as well, rather than a separate println here?
412: You're passing back hsStatus, but not using it. Suggest void.
Hope this helps.
Brad
On 9/21/2018 2:35 PM, Jamil Nimeh wrote:
Hello all,
This adds a test that lets us send different kinds of client hellos to a
JSSE server. It can be extended to handle different kinds of corner
cases for client hello extension sets as well as fuzzing test cases in
the future. It also provides some extra test coverage for JDK-8210334
and JDK-8209916.
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210918/webrev.01/
JBS: https://bugs.openjdk.java.net/browse/JDK-8210918
Thanks and have a good weekend,
--Jamil
