LadyForest commented on code in PR #24390:
URL: https://github.com/apache/flink/pull/24390#discussion_r1507195791
##########
flink-connectors/flink-connector-files/src/test/java/org/apache/flink/connector/file/table/FileSystemOutputFormatTest.java:
##########
@@ -104,15 +113,101 @@ void testNonPartition() throws Exception {
.containsExactly("a1,1,p1\n" + "a2,2,p1\n" + "a2,2,p2\n" +
"a3,3,p1\n");
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ void testStagingDirBehavior(boolean shareStagingDir) throws Exception {
Review Comment:
> Generally speaking, unit tests should be as small as possible to focus on
the specific aspect of the class contract (in this case
`FileSystemTableSink#toStagingPath` not returning the same folder twice. This
test adds quite a bit of extra context here which might make it harder to grasp
the intention for code readers. What's your opinion on that?
+1 with your perspective on unit testing. As I previously mentioned, in this
case, constructing a UT is quite challenging, which subtly suggests to me that
there may be some design issues with the current code. Take the staging dir,
for example, if it needs to be passed from the `FileSystemTableSink` to the
`OutputFormat` at compile-time, then a proper validation should be done here.
Conversely, perhaps the `OutputFormat` could decide on the stagingDir itself,
which would make the testing more targeted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
