Fix #3730 improve paho-mqtt5 ssl tests

[zhfeng]

No description provided.

[ppalaga]

I still wonder why removing the keyStore is important. Could you please explain the motivation? Would other tests' expectations in this Maven module somehow clash with those system props being set permanently?
Tests in other modules should not matter because surefire always bootstraps a new JVM for running tests within a module.

If the keystore removal is really important, than we should perhaps enclose receiveBody in a try block and call removeKeyStore() in finally like you do below.

[zhfeng]

Well, @jamesnetherton suggest to unset the keystore properties in #3709 (comment) and I think it is reasonable to clean the environment properties after testing. I'm not very sure that surefire always lauchs a new JVM for each test ? e.g. with reuseForks=true

[ppalaga]

My understanding of reuseForks is that it controls whether JVMs can be reused for individual test classes within the same Maven module.

To make sure, I have added

System.out.println("=== pid(" + getClass().getSimpleName() + ") " + ManagementFactory.getRuntimeMXBean().getName());

to Base64Test and BeanValidatorTest and I have run

cd integration-tests && mvn clean test -pl :camel-quarkus-integration-test-base64,:camel-quarkus-integration-test-bean-validator
...
=== pid(Base64Test) 111907@terpistone
...
=== pid(BeanValidatorTest) 112141@terpistone

So the two tests from two different Maven modules were run in two different JVMs.

it is reasonable to clean the environment properties after testing.

+1 on generally wanting to cleanup after the test. The questions in this particular case are:

1. Whether the cleanup does what it is supposed to do
2. Whether there might be any unwanted side effects.
3. Whether this is a good practice to be followed by end users

For 1., it depends how we define our intention. Removal of the system props themselves is effective for sure, but I doubt that by doing that we also remove the SSL context instantiated somewhere deep in Paho. Should that be our goal? I don't know, probably not. But if we do not remove the SSL context whose state relies on the props, should we remove the props (and the key store file) at all? Maybe better not before the the test app is shut down?

For 2., javax.net.ssl.* properties are not owned by us. They are owned by the JVM. There may exist third party code sensitive to the changes. My impression from what I have read about javax.net.ssl.* in the past is, that they are intended to be passed to a JVM at startup and that they are not so much intended to be updated at runtime. Will we break any third party code relying on javax.net.ssl.* properties? Probably not (otherwise we'd see failures in the test).

For 3., I'd say our end users should not be setting javax.net.ssl.* properties at runtime for reasons named in 2. Removing the key store file and the props may break something in their app. If we keep the code in the test, I think we should at least add a comment saying something like "This may break code relying on javax.net.ssl.* properties. Do not do this in production.".

Maybe passing the props from a TestResource to the app when it is starting and removing the key store file after the app is shut down from QuarkusTestResourceLifecycleManager.stop() would address all these questions?

[zhfeng]

Thanks a lot @ppalaga! I revisit the paho-mqtt5-component.sslClientProps and it seems that we can pass them with com.ibm.ssl.keyStore and com.ibm.ssl.keyStorePassword in uri. It should be better to avoid setting javax.net.ssl.* system properties. I will try it at first.

[zhfeng]

yeah, it works.

[ppalaga]

Great work, thank, @zhfeng !

[aldettinger]

Great to see this improvement. Nice jobs @ppalaga and @zhfeng 👍