ARTEMIS-5340 ensure PEM provider is truly optional #5547
Conversation
...ient/src/test/java/org/apache/activemq/artemis/core/remoting/impl/netty/PEMProviderTest.java
Outdated
Show resolved
Hide resolved
...core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/SSLSupport.java
Outdated
Show resolved
Hide resolved
jbertram
force-pushed
the
ARTEMIS-5340
branch
3 times, most recently
from
0edfed3 to
81de28f
Compare
| // ensure the PEM provider wasn't already loaded by some other test | ||
| Assumptions.assumeFalse(Arrays.stream(ClassLoader.getSystemClassLoader().getDefinedPackages()).anyMatch(pkg -> PEM_PROVIDER_PACKAGE.equals(pkg.getName()))); |
There was a problem hiding this comment.
Would be better to make it an assertion and let it fail when it needs to, to serve as notice the two tests are clashing and need to be ordered or isolated from each other to fix the problem.
With an assumption, if some other new test is created and runs first, then this test will just start skipping and the 'dont load it to let it be removed if not used' effect the test verifies could be broken without noticing, unless someone spots the extra skip beginning to occur. Might as well not have a test at that point.
Its a requirement for the test to exist that it be run before the first other test in the module to use it, but its not a reason to optionally run the test or not. We always want to run it, but should check the requirement was satisfied at the start, the same way the test checks it at the end currently.
| public void testPemProviderPackageName() { | ||
| assertEquals(PEM_PROVIDER_PACKAGE, PemKeyStoreProvider.class.getPackageName()); | ||
| assertNotNull(ClassLoader.getSystemClassLoader().getDefinedPackage(PEM_PROVIDER_PACKAGE)); | ||
| } |
There was a problem hiding this comment.
I think a better test would be loading the provider the way its normally to be loaded, by using SSLSupport to load a pem keystore (the other test could also load a non-pem keystore to similarly check that does not load the pem provider), instead of just directly loading the class.
The 'check the correct package is looked for' could still be done, just as the last thing the test does as a form of confirmation. Or it could be a third test.
There was a problem hiding this comment.
I added a couple of new tests to verify loading keystores in addition to the static use.
|
LGTM |
No description provided.