snazy commented on code in PR #4472:
URL: https://github.com/apache/polaris/pull/4472#discussion_r3267871289
##########
persistence/nosql/persistence/testextension/src/main/java/org/apache/polaris/persistence/nosql/testextension/PersistenceTestExtension.java:
##########
@@ -165,11 +165,11 @@ public boolean supportsParameter(
}
private void assertValidFieldCandidate(Field field) {
- if (!field.getType().isAssignableFrom(SnowflakeIdGenerator.class)
- && !field.getType().isAssignableFrom(MonotonicClock.class)
- && !field.getType().isAssignableFrom(Persistence.class)
- && !field.getType().isAssignableFrom(Backend.class)
- && !field.getType().isAssignableFrom(BackendTestFactory.class)) {
+ if (!IdGenerator.class.isAssignableFrom(field.getType())
+ && !MonotonicClock.class.isAssignableFrom(field.getType())
Review Comment:
Well, yes and no. And also no to that the fix here as you reviewed is
correct.
Honestly, any `isAssignableFrom` is wrong, especially considering what
`resolve()` does.
The underlying issue is that the instances produced for parameters and
fields are created from factories.
So technically we would have to do an `isAssignableFrom` against the
actually produced type. But those produced instances already allocate
resources, which is a bit heavy for a simple "supports this parameter type"
check.
So I took the easy route and replaced the `isAssignableFrom` checks with a
straight `==` against the `Class`es, which is fine for the current tests. If we
need more later, we can add it. But this approach feels much safer - and it's
for tests, failing early and hard is fine there.
--
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]
