Hi Artem,
thank you for your comments. Please see updated review:
http://cr.openjdk.java.net/~snikandrova/8136933/webrev.01/
<http://cr.openjdk.java.net/%7Esnikandrova/8136933/webrev.01/>
I've moved version check to OSInfo.java and used ProcessTools. Also
made a little change in ProcessTools (Throwable didn't make sense
there). I also updated comment for checkSocketOption() method to
indicate that it is fine to not get any exception. Actually as this test
is meant to check platform support we are only interested in
UnssuportedOperationException. More usage scenarios with SO_FLOW_SLA
option are covered by
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/tip/test/jdk/net/Sockets/Test.java
<http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/tip/test/jdk/net/Sockets/Test.java>Thank
you,
Svetlana
On 19.05.2016 19:51, Artem Smotrakov wrote:
Hi Svetlana,
Please see a couple of comments below. I'll leave the final review to
official reviewers.
1. You may use test/lib/testlibrary/jdk/testlibrary/OSInfo.java to
check OS type. I think it also may be good to add getSolarisVersion()
to OSInfo.java (see getWindowsVersion() method)
You can also use ProcessTools.java in getSolarisVersion()
2. line 46: Did you mean java.lang.String
3. Should it throw an exception after lines 173 and 181? Or, this may
depend on other constraints? I see you added a comment for
checkSocketOption() method, but it maybe better to clarify why no
exception is also fine.
I am also wondering if it should expect no exception in getOption() if
setOption() succeeded.
Artem
On 05/19/2016 09:13 AM, Svetlana Nikandrova wrote:
Hello,
please review additional test for Solaris SO_FLOW_SLA socket option.
Test checks that SO_FLOW_SLA option is supported on Solaris 11.2+ and
not supported on other platforms.
http://cr.openjdk.java.net/~snikandrova/8136933/webrev.00/
<http://cr.openjdk.java.net/%7Esnikandrova/8136933/webrev.00/>
Thank you,
Svetlana
