Add Kudu unit test class against latest 1.14.0 #7127
Conversation
| import static io.trino.spi.type.VarcharType.VARCHAR; | ||
| import static io.trino.testing.MaterializedResult.resultBuilder; | ||
| import static io.trino.testing.assertions.Assert.assertEquals; | ||
|
|
||
| public class TestKuduDistributedQueries | ||
| public abstract class BaseKuduDistributedQueries |
There was a problem hiding this comment.
Can we instead use base BaseKuduDistributedQueries no BaseConnectorTest?
There was a problem hiding this comment.
Added an additional commit to extend BaseConnectorTest.
adamjshook
force-pushed
the
kudu-latest
branch
2 times, most recently
from
d8f4744 to
f2eca9d
Compare
| public abstract class BaseKuduDistributedQueries | ||
| extends AbstractTestDistributedQueries | ||
| public abstract class BaseKuduConnectorTest | ||
| extends BaseConnectorTest |
There was a problem hiding this comment.
If you are using BaseConnectorTest then you should also remove AbstractKuduIntegrationSmokeTest as smoke tests are already in BaseConnectorTest.
There was a problem hiding this comment.
@kokosing Took a look at removing this. I am a bit hesitant because each of the integration smoke tests implementations have a separate schema inference -- disabled, empty, or standard -- which is given to as an input parameter to a QueryRunner. This seems intentional to me, but I am not familiar with the Kudu connector tests. Just trying to add support for testing the latest. Let me know your thoughts.
I've rebased this PR to the latest master.
There was a problem hiding this comment.
each of the integration smoke tests implementations have a separate schema inference -- disabled, empty, or standard
So you can have a dedicated instance ofBaseKuduConnectorTestfor each case. Or you can create aBaseKuduConnectorSmokeTest(that extendsBaseConnectorSmokeTest) and use it for each case.
This removes test coverage but it is restored in the next commit. We want to retain the history on the file changes.
| import static io.trino.plugin.kudu.KuduQueryRunnerFactory.createKuduQueryRunnerTpch; | ||
|
|
||
| public class TestKuduLatestConnectorTest | ||
| extends BaseKuduConnectorTest |
There was a problem hiding this comment.
Should this be a ConnectorSmokeTest only?
There was a problem hiding this comment.
I've been a bit out of the loop on the changes from Distributed/Smoke tests to these BaseConnectorTest and BaseConnectorSmokeTest. I'm okay keeping the "latest" tests as a smoke test if that has been the pattern that has been adopted for testing the later versions of the software. Makes sense since the expectation is that newer versions should work and we don't need to go through the full breadth of testing and a quick smoke test is sufficient.
|
Let me close this PR because #10940 fixed the tests. |
This commit refactors the distributed tests into a base with two implementations, the existing unit tests against the 1.10.0 of Kudu and the latest 1.14.0 version.
Relates to #5804