Skip to content
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

Merged
merged 10 commits into from
Sep 27, 2019

Conversation

purplefox
Copy link
Contributor

@purplefox purplefox commented Sep 26, 2019

Description

Fixes #2804

  • This PR breaks dependencies from ksql-functional-tests (jar) to any test-jars.
  • I've refactored the code that creates the ServiceContext and KSqlEngine a little to reduce the coupling with test-jar code.
  • I've duplicated some matcher classes - I've put these in their own directory
  • I've duplicated some Fake* classes- I've put these in their own directory

Testing done

Ran the full test suite

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@purplefox purplefox requested a review from a team as a code owner September 26, 2019 13:26
@purplefox purplefox changed the title Fix 2804 Fix: Remove dependencies on test-jars from ksql-functional-tests jar Sep 26, 2019
@purplefox purplefox changed the title Fix: Remove dependencies on test-jars from ksql-functional-tests jar fix: Remove dependencies on test-jars from ksql-functional-tests jar Sep 26, 2019
Copy link
Contributor

@agavra agavra left a 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

@agavra agavra requested a review from a team September 26, 2019 18:59
@purplefox
Copy link
Contributor Author

Show functions gives me this, I don't think it contains any test functions:

ksql> show functions;

 Function Name         | Type
-----------------------------------
 ABS                   | SCALAR
 ARRAYCONTAINS         | SCALAR
 AS_ARRAY              | SCALAR
 AS_MAP                | SCALAR
 AVG                   | AGGREGATE
 CEIL                  | SCALAR
 COLLECT_LIST          | AGGREGATE
 COLLECT_SET           | AGGREGATE
 CONCAT                | SCALAR
 COUNT                 | AGGREGATE
 DATETOSTRING          | SCALAR
 ELT                   | SCALAR
 EXP                   | SCALAR
 EXTRACTJSONFIELD      | SCALAR
 FIELD                 | SCALAR
 FLOOR                 | SCALAR
 GEO_DISTANCE          | SCALAR
 HISTOGRAM             | AGGREGATE
 IFNULL                | SCALAR
 INITCAP               | SCALAR
 LCASE                 | SCALAR
 LEN                   | SCALAR
 LN                    | SCALAR
 MASK                  | SCALAR
 MASK_KEEP_LEFT        | SCALAR
 MASK_KEEP_RIGHT       | SCALAR
 MASK_LEFT             | SCALAR
 MASK_RIGHT            | SCALAR
 MAX                   | AGGREGATE
 MIN                   | AGGREGATE
 RANDOM                | SCALAR
 REPLACE               | SCALAR
 ROUND                 | SCALAR
 SIGN                  | SCALAR
 SLICE                 | SCALAR
 SPLIT                 | SCALAR
 SQRT                  | SCALAR
 STRINGTODATE          | SCALAR
 STRINGTOTIMESTAMP     | SCALAR
 SUBSTRING             | SCALAR
 SUM                   | AGGREGATE
 SUM_LIST              | AGGREGATE
 TIMESTAMPTOSTRING     | SCALAR
 TOPK                  | AGGREGATE
 TOPKDISTINCT          | AGGREGATE
 TRIM                  | SCALAR
 UCASE                 | SCALAR
 UNIX_DATE             | SCALAR
 UNIX_TIMESTAMP        | SCALAR
 URL_DECODE_PARAM      | SCALAR
 URL_ENCODE_PARAM      | SCALAR
 URL_EXTRACT_FRAGMENT  | SCALAR
 URL_EXTRACT_HOST      | SCALAR
 URL_EXTRACT_PARAMETER | SCALAR
 URL_EXTRACT_PATH      | SCALAR
 URL_EXTRACT_PORT      | SCALAR
 URL_EXTRACT_PROTOCOL  | SCALAR
 URL_EXTRACT_QUERY     | SCALAR
 WINDOWEND             | AGGREGATE
 WINDOWSTART           | AGGREGATE
-----------------------------------

Copy link
Contributor

@big-andy-coates big-andy-coates left a 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(
Copy link
Contributor

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.

Copy link
Contributor Author

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

<groupId>io.confluent.ksql</groupId>
<artifactId>ksql-engine</artifactId>
<version>${project.version}</version>
<type>test-jar</type>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

@purplefox
Copy link
Contributor Author

@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).
Very hacky but something like this at startup of the server

boolean isRunningTestsuite = cl.getResource("Name of some Junit class") != null;
if (!isRunningTestsuite && cl.getResource("Name of some test udf class") != null) {
    // bail out
}

@purplefox purplefox merged commit e09d6ad into confluentinc:master Sep 27, 2019
@purplefox purplefox deleted the fix-2804 branch September 30, 2019 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ksql-functional-test module depends on test-jars
3 participants