[WIP] #44 Integration tests setup

[iabudiab]

Hey there 👋 I am working on a setup for integration tests with testcontainers. The first test runs w/o problems (on my machine 😁)

However, this is a WIP right now because I wanted to ping-pong some details here:

• One way to test the commands is to assert the return codes and their outputs. Using the @Spec CommandSpec from picocli allows for easier testing. See the changes in the InfoCommand in this PR. Basically a CommandSpec can be injected vie the @Spec annotation and then used for writing the output. In the test we can then set the output stream of the CommandLine like this and then assert on it:

CommandLine commandLine = new CommandLine(infoCommand);
StringWriter output = new StringWriter();
commandLine.setOut(new PrintWriter(output));

• A better way would be to assert on the JSON (Output formats #98) instead of the "human" format. So maybe I could focus the efforts there before finishing this one.

• The commands should use constructor injection instead of field injection for them to be testable. i.e.

@Inject
public InfoCommand(ConfigurationContext context) {
     this.context = context;
}

• Better yet, the whole RestClient should be somehow injectable. That way we could write unit and integration tests. Right now only blackbox testing is easily doable (I think), because the client is initialised when the command runs. Maybe someone with more quarks experience could chime in here 😉

• Do we have some guidelines/conventions for the status codes and the output streams? i.e. out vs err? I am asking, because for example what would be the better approach for Handle attempt to delete non-existing connector gracefully #83? Status 0 and stdout? or some non-zero status with stderr? or status 0 but stderr?

• I've used a @Tag for the integration test, in order to be able to run them selectively:

# run all tests
mvn test

# run only integration tests
mvn test -Dgroups=integration-test

# run all except integration tests
mvn test -Dgroups=!integration-test

What do you think?

[gunnarmorling]

One way to test the commands is to assert the return codes and their outputs. Using the @SPEC CommandSpec from picocli allows for easier testing. See the changes in the InfoCommand in this PR. Basically a CommandSpec can be injected vie the @SPEC annotation and then used for writing the output. In the test we can then set the output stream of the CommandLine like this and then assert on it

Ah, that's cool, didn't know about this. Seems like the way to go; in fact, we already have some tests where we redirect the output stream, ideally we'd rework those then to use the spec approach.

A better way would be to assert on the JSON

You mean better as it'd be easier to parse the output and retrieve specific bits of information? If so, that sounds good too. OTOH testing the human-readable output would also be valuable. Both would work for me.

In regards to (constructor) injection of dependencies, I'll be happy with everything which helps to add some tests. That said, I'm not a big believer into mocked tests, so I think my preference would always be integration tests which actually test against a running Kafka Connect. Note with offset display and reset (#2, #8), we'll also interact with Kafka proper, which would make mocking even more complex.

Do we have some guidelines/conventions for the status codes and the output streams? i.e. out vs err?

Good question; no explicit guidelines so far. We should return a value other than 0 if there's an error (that should be the case already pretty consistently; where it isn't we should change it). In terms of stdout vs. stderr, I have no preference tbh. Will contents from stderr be visible by default on the console? I've only ever used stdout so far. Replied on #32, too.

I've used a @tag for the integration test, in order to be able to run them selectively

Ah yes, that's very nice.

Thanks a lot for driving this forward, @iabudiab, and welcome to the project!

[gunnarmorling]

This looks great overall; a few comments inline. Thanks a lot, @iabudiab!

[gunnarmorling]

Do we need core actually?

[iabudiab]

I'm not 100% sure. If I remember correctly it is/was required for some version. I'll check and remove accordingly if it's not the case.

[gunnarmorling]

So can we get this via the constructor too? Would be nice if we could make all the fields private final.

[iabudiab]

Not easily, since then we would have to register picocli CommandSpecs for quarkus to somehow be injectable. I could take a deeper look into that.

[gunnarmorling]

Ok, I see. No worries, I think we can go with that. But worth opening an issue against the Quarkus PicoCLI extension for getting that supported OOTB at some point.

[iabudiab]

I've looked into this a little bit. However, I couldn't find an easy/direct way to make this work with Quarkus. It seems that supporting this in PicoCLI directly is the best option.

[iabudiab]

Thanks for the feedback and the warm welcome 😄 I'll pick up and continue this PR sometime in the next couple of days.

[ThomasDangleterre]

Maybe you could implement QuarkusTestResourceLifecycleManager ?

[gunnarmorling]

+1, that's a great idea.

[gunnarmorling]

Any thoughts on this one, @iabudiab?

[iabudiab]

I've made a PoC implementation of this on a separate branch in case this is the way we want to go forward. However, I wasn't really happy with the way it integrates with JUnit and the glue it requires. And because of my limited time and Quarkus experience, I decided not pursue it further at this point. Of course, if we decide that this is nonetheless the better way, then we have that option too ;)

Please see my comment for more details: #110 (comment)

[helpermethod]

Hi @gunnarmorling, with regards to interacting with Kafka:

I had very good results testing the interacting with MockProducer and MockConsumer. Those tests are really fast and don't need a running Kafka instance.

You can still have a smaller number of integrations tests with Testcontainers to test the real thing.

[iabudiab]

👋 just wanted to check-in and let you all know, I am still working on this, however the progress is really slow because I am on (long overdue) vacation now 😸
I won't be able to make real progress until after Christmas 🎄☃️
Cheers and happy holidays everybody

[gunnarmorling]

Hey @iabudiab, thanks for the heads-up. No worries, looking forward to this one, once you find the time for it. Have a great vacation, and Happy Holidays to you, too!

[iabudiab]

Happy new year everybody!

So, here is a summary of the current state of this PR and some of the gotchas I collected while on a trip down the rabbit hole :) (This is my first time using quarkus)

Stuff good to know to reduce the time spent (re)searching (in no particular order):

• Quarkus and JUnit5 integration is not optimal. For example you can't use JUnit5 extensions with @QuarkusTests because quarkus uses a separate class loader when instantiating a test instance => The test instance in the extension is not the same that is actually used for the test, so that almost everything you do in the extension is not seen in the test.
• Using io.debezium:debezium-testing-testcontainers version > 1.6.x requires io.debezium:debezium-core:test-jar because of this one class io.debezium.util.ContainerImageVersions. It should either bring the dependency transitively or this class should be refactored somewhere else maybe? @gunnarmorling maybe you can sort it out on the Debezium side?
• quarkus-universe-bom pins com.squareup.okhttp3:okhttp to version 3.14.9, which is fine, unless you are using DebeziumContainer in order to create connectors in the running container. io.debezium:debezium-testing-testcontainers requires at least okhttp:4.x. So without okhttp:4.x dependency you would get NoSuchMethodErrors and NoSuchFieldError when using certain methods from DebeziumContainer, e.g. registering a connector.
• Quarkus test callbacks (i.e. AfterTestCallback, BeforeEachTestCallback etc.) can only be deployed as service provider on the class path, which would activate them for all the tests. There is currently no way (from what I've seen) to have test callbacks analogous to JUnit5 extension, i.e. activatable via annotations, when using @QuarkusTests.

This is why I chose to implement the simplest options because of the limited time that I have, i.e. an abstract super class for integration tests, that contains all the required utilities and where all subclasses can access stuff where needed.

However, I pushed the other two options, that I've tried onto separate branches, in case somebody with more quarkus experience than me wants to pick those up and continue from there or give some pointers 😉. You can find those here: quarkus_test_resource and here: junit_extension. Take a look at the test class org.kcctl.command.ApplyComamnd and related code.

I've implemented some integration tests now for several commands and will push the rest whenever I get the time.

Cheers.

[gunnarmorling]

Hey @iabudiab, some very small comments inline, but overall this looks awesome! Thank you so much for this excellent piece of work, it's a huge step forward for the project to have this test suite.

[iabudiab]

@gunnarmorling I'm glad to help 😄 I wished I had more free time to move things along faster.

[gunnarmorling]

It should either bring the dependency transitively or this class should be refactored somewhere else maybe? @gunnarmorling maybe you can sort it out on the Debezium side?

Took a look into this, and the dependency is already established correctly by debezium-testing-testcontainers. The problem is though that the Quarkus BOM brings in debezium-core 1.6.1, whereas here we explicitly depend on debezium-testing-testcontainers 1.8.0. So I have pinned the version for core to 1.8.0 via the dependency management within kcctl, and then things work as they should, without adding the transitive core dependency.

quarkus-universe-bom pins com.squareup.okhttp3:okhttp to version 3.14.9, which is fine, unless you are using DebeziumContainer in order to create connectors in the running container.

Ah, that's interesting. Thinking about it, I don't think we should use okhttp in Debezium to begin with, but rather use the HTTP client coming with Java 11+. I've logged https://issues.redhat.com/browse/DBZ-4594 for this.

Regarding the Quarkus integration issues you encountered, I think the base class approach is good enough. I'll log a follow-up issue, referencing the branches you linked to above, in case you or someone else will have the time to look into it. It'd be a bit more "idiomatic" and doing things the Quarkus way, but it doesn't really change anything in terms of functionality, and the tests you added provide excellent value as-is.

I have rebased everything to the latest main to resolve some merge conflicts and added a few fixes. All this is in PR #144, which supersedes this one. I'll close this and merge the other one in second. Thanks a lot, @iabudiab, this is really a fantastic contribution to kcctl, substantially improving the trust we can have into future changes, and helping to spot any regressions. Thank you!