Patrick,
On 24/01/2020 11:14, Patrick Concannon wrote:
Hi,
Could someone please review my fix and CSR for issue JDK-7021373
'DatagramPacket exception conditions are not clear' ?
This fix updates the spec concerning the exceptions thrown by the
constructors of the DatagramPacket class, and several methods therein.
bug: https://bugs.openjdk.java.net/browse/JDK-7021373
CSR: https://bugs.openjdk.java.net/browse/JDK-8237774
webrev: http://cr.openjdk.java.net/~pconcannon/7021373/webrevs/webrev.00/
This is mostly good. Some pedantic comments:
1) Let's add coverage for NPE for ALL constructors, e.g.
@Test
public void testNull() {
expectThrows(NPE, () -> new DatagramPacket(null, 100));
expectThrows(NPE, () -> new DatagramPacket(null, 0, 10));
expectThrows(NPE, () -> new DatagramPacket(null, 0, 10,
LOOPBACK, 80));
expectThrows(NPE, () -> new DatagramPacket(null, 10, LOOPBACK, 80));
expectThrows(NPE, () -> new DatagramPacket(null, 0, 10, new
InetSocketAddress(80)));
expectThrows(NPE, () -> new DatagramPacket(null, 10, new
InetSocketAddress(80)));
}
2) I personally dislike the use of "data buffer" here. The invariants
are checked on the "given" buffer. The given buffer only has an
affect on the packet after the invariants have been checked.
3) Setters - this is a nice test. Some suggestions to improve
readability:
a) line up multi-line parameters vertically, e.g.
57 @Test(dataProvider = "data")
58 public void testSetData(byte[] buf,
int offset,
int length,
59 Class<? extends Exception> exception) {
b) line up the provider's data to improve readability, e.g.
95 return new Object[][]{
96 { -1, IAE },
97 { -666, IAE },
98 { Integer.MAX_VALUE, IAE },
99 { 0xFFFF + 1, IAE },
100 { 0xFFFFFF, IAE },
101 { 0, null },
102 { 1, null },
103 { 666, null },
104 { 0xFFFE, null },
105 { 0xFFFF, null },
106 };
4) Add a NPE test for setData(byte[] buf) - I think only the 3-arg
overload is tested?
-Chris.
