-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Remove dependencies on test-jars from ksql-functional-tests jar #3421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! Can you also test to make sure that the test UDFs no longer show up in the SHOW FUNCTIONS
when we run KSQL? If this fixes that issue then some code duplication is totally okay IMO. Let's get two +1s on this just to get some consensus, maybe @big-andy-coates
Show functions gives me this, I don't think it contains any test functions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @purplefox
Great to see this resolved!
I'm wondering if it's possible to add a test somewhere that will fail should these things get back on the classpath. That would be the icing on the cake. Unfortunately, I think the only place such a test is actually valid would be in our system tests.
I'm a little wary of adding SqlFormatInjector
into the production code base when its only used for tests... @agavra, does it make sense to actually use this injector in our normal flow? So that we always reformat statements? If so, @agavra, do you think you could knock out a quick follow up PR to this one to make that the case?
@@ -80,7 +80,7 @@ public KsqlEngine( | |||
)); | |||
} | |||
|
|||
KsqlEngine( | |||
public KsqlEngine( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this change is not needed. It would be good to keep this package private and, if you're game, annotate with @VisibleForTestingOnly
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestExecutor.getKsqlEngine() requires this to be public
ksql-engine/src/main/java/io/confluent/ksql/services/SandboxedTopicDescription.java
Outdated
Show resolved
Hide resolved
ksql-engine/src/main/java/io/confluent/ksql/services/TopicValidationUtil.java
Outdated
Show resolved
Hide resolved
<groupId>io.confluent.ksql</groupId> | ||
<artifactId>ksql-engine</artifactId> | ||
<version>${project.version}</version> | ||
<type>test-jar</type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!
...nctional-tests/src/main/java/io/confluent/ksql/test/tools/stubs/FakeKafkaClientSupplier.java
Outdated
Show resolved
Hide resolved
@big-andy-coates In order to test that the test udfs haven't got back on the classpath there is a way we could do it without having to have a reference to the actual class or to be running as an actual test (where the test udf would be there anyway).
|
Description
Fixes #2804
Testing done
Ran the full test suite
Reviewer checklist