RussellSpitzer commented on code in PR #13339:
URL: https://github.com/apache/iceberg/pull/13339#discussion_r2154997143
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -456,6 +462,39 @@ public void testCloseChild() throws IOException {
.doesNotThrowAnyException();
}
+ @ParameterizedTest(name = "[{index}] selfConflict={0}")
+ @ValueSource(booleans = {true, false})
Review Comment:
This is more of a style issue but I think in general we shouldn't
parameterize a test like this if the bodies are substantially different
(especially if the check parts are different.)
The way I read this now is
```
common setup
Branch A - One Step
common request
Branch A Validation
Else Branch B Validation
```
So the two tests don't really share a lot
I think it would be easier to read if we just split it into two different
tests.
testCommitConflict
testRetryAndCommitConflict
We could also think about refactoring addRequestTestCaseAndGetPath to take a
container object to avoid the common setup but I don't think that's a huge deal.
--
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...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
