nastra commented on code in PR #5984:
URL: https://github.com/apache/iceberg/pull/5984#discussion_r1000185608
##########
api/src/test/java/org/apache/iceberg/AssertHelpers.java:
##########
@@ -48,6 +48,22 @@ public static void assertThrows(
}
}
+ /**
+ * A convenience method to avoid a large number of @Test(expected=...) tests
+ *
+ * @param expected An Exception class that the Runnable should throw
+ * @param containedInMessage A String that should be contained by the thrown
exception's message
+ * @param callable A Callable that is expected to throw the exception
+ */
+ public static void assertThrows(
Review Comment:
I think it would be better to use `Assertions` from AssertJ directly
(because it gives you greater flexibility) rather than adding new stuff to
`AssertHelpers`. And in order to improve the issue around the fully-qualified
import due to a conflict with `TableTestBase.Assertions`: I'll open a PR and
will rename `TableTestBase.Assertions` to something that doesn't conflict with
`Assertions` from AssertJ
##########
core/src/main/java/org/apache/iceberg/TableScanContext.java:
##########
@@ -109,7 +113,30 @@ TableScanContext useSnapshotId(Long scanSnapshotId) {
toSnapshotId,
planExecutor,
fromSnapshotInclusive,
- metricsReporter);
+ metricsReporter,
+ ref);
+ }
+
+ String branch() {
Review Comment:
should this also be named `ref()`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
