flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] pnowojski commented on a change in pull request #6705: [FLINK-10356][network] add sanity checks to SpillingAdaptiveSpanningRecordDeserializer
Date Fri, 01 Mar 2019 14:20:20 GMT
pnowojski commented on a change in pull request #6705: [FLINK-10356][network] add sanity checks
to SpillingAdaptiveSpanningRecordDeserializer
URL: https://github.com/apache/flink/pull/6705#discussion_r261614343
 
 

 ##########
 File path: flink-runtime/src/test/java/org/apache/flink/runtime/io/network/api/serialization/SpanningRecordSerializationTest.java
 ##########
 @@ -105,11 +119,235 @@ public void testHandleMixedLargeRecords() throws Exception {
 		testSerializationRoundTrip(originalRecords, segmentSize);
 	}
 
+	/**
+	 * Non-spanning, deserialization reads one byte too many and succeeds - failure report comes
+	 * from an additional check in {@link SpillingAdaptiveSpanningRecordDeserializer}.
+	 */
+	@Test
+	public void testHandleDeserializingTooMuchNonSpanning1() throws Exception {
+		testHandleWrongDeserialization(
+			DeserializingTooMuch.getValue(),
+			32 * 1024);
+	}
+
+	/**
+	 * Non-spanning, deserialization reads one byte too many and fails.
+	 */
+	@Test
+	public void testHandleDeserializingTooMuchNonSpanning2() throws Exception {
+		testHandleWrongDeserialization(
+			DeserializingTooMuch.getValue(),
+			(serializedLength) -> serializedLength,
+			isA(IndexOutOfBoundsException.class));
 
 Review comment:
   Did you mean `Exception` instead of `IOException`? I think that we should either clean
this up and throw only one type of an exception (`DeserialisationException`?) or if we are
throwing "unspecified" exceptions, we should test just for that (`isA(Exception.class)`).
   
   Again I would like to avoid the scenario when someone in the future makes a good refactor/clean
up, one of those tests starts to fail, and our future programmer will have to spend time thinking
whether throwing exactly `IndexOutOfBoundsException` in one case and `EOFException` in other
was part of some important contract. This is what I meant by "codifying a contract". Such
kind of tests might cause someone to tip-toe/triple think "what is happening here?".

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message