Refactor JUnit 5 tests consolidate BaseTestRSA#1194
Refactor JUnit 5 tests consolidate BaseTestRSA#1194Mohit-Rajbhar100698 wants to merge 1 commit intoIBM:mainfrom
Conversation
|
We did talk about this update some and found the runtimes for the test buckets increased by ~4x. Some additional work here to only run 2048 base keys for multi-thread is being tried. |
Mohit-Rajbhar100698
force-pushed
the
mohit-rajbhar/junit5-BaseTestRSA-consolidation
branch
from
ea991d6 to
22b8ad7
Compare
This change consolidates various tests associated with the former BaseTestRSA into a single parameterized test. Fixes: https://github.ibm.com/runtimes/jit-crypto/issues/1063 Signed-off-by: Mohit Rajbhar <mohit.rajbhar@ibm.com>
Mohit-Rajbhar100698
force-pushed
the
mohit-rajbhar/junit5-BaseTestRSA-consolidation
branch
from
22b8ad7 to
87c6137
Compare
|
Since a @MethodSource cannot be overridden using inheritance, we cannot simply extend TestRSA and change the method source to run only the 2048 key size. I created two separate test classes. One class runs all RSA key sizes except 2048, and the other class runs only the 2048 key size for multithread testing. Both classes extend the same base test class (TestRSA), which contains all the common setup and tests. |
| import static org.junit.jupiter.api.Assumptions.assumeTrue; | ||
|
|
||
| public class BaseTestRSA extends BaseTestCipher { | ||
| public abstract class TestRSA extends BaseTestCipher { |
There was a problem hiding this comment.
Since this is an abstract class and we expect other classes to extend it can we keep the naming convention of BaseTestRSA for this class?
| @Tag(Tags.OPENJCEPLUS_NAME) | ||
| @Tag(Tags.OPENJCEPLUS_FIPS_NAME) | ||
| @Tag(Tags.OPENJCEPLUS_MULTITHREAD_NAME) | ||
| @Tag(Tags.OPENJCEPLUS_FIPS_MULTITHREAD_NAME) | ||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
| @ParameterizedClass | ||
| @MethodSource("ibm.jceplus.junit.tests.TestArguments#rsaKeySize2048AndJCEPlusProviders") | ||
| public class TestRSA_2048 extends TestRSA { |
There was a problem hiding this comment.
This test is intended to only run multhreaded given that it only ( for now to match past behavior ) runs 2048 sized keys. I believe we want to remove these tags from the test so it is not run for our regular openjceplus and openjceplusfips test suites:
@Tag(Tags.OPENJCEPLUS_NAME)
@Tag(Tags.OPENJCEPLUS_FIPS_NAME)
And also rename the test to something like TestRSAMultiThread since this is a multithreaded specific test. I dont think we need to put the keysize in the test name given we may want to in the future add more key sizes to the parameters of this test.
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
| @ParameterizedClass | ||
| @MethodSource("ibm.jceplus.junit.tests.TestArguments#rsaKeySizesAndJCEPlusProviders") | ||
| public class TestRSA_Non2048 extends TestRSA { |
There was a problem hiding this comment.
This tests tags seem correct. We do however want to run all the available key sizes for RSA in the test so it runs as part of the openjceplus or openjceplusfips test suites. The class can also be simply named TestRSA.
| if (keySize == 512) { | ||
| specifiedKeySize = 512; | ||
| } else if (keySize == 1024) { | ||
| specifiedKeySize = 1024; | ||
| } else if (keySize == 2048) { | ||
| specifiedKeySize = 2048; | ||
| } else if (keySize == 3072) { | ||
| specifiedKeySize = 3072; | ||
| } else if (keySize == 4096) { | ||
| specifiedKeySize = 4096; | ||
| } else { | ||
| throw new Exception("Illegal key size in BaseTestRSA."); | ||
| } |
There was a problem hiding this comment.
Could we eliminate the specifiedKeySize variable here and just make use of keySize in the test?
| protected abstract int getInputKeySize(); | ||
|
|
||
| protected abstract TestProvider getInputProvider(); |
There was a problem hiding this comment.
Are these abstract methods required? Can we simply just use the keySize and provider that is set by the parameterized classes that extend this abstract class?
|
|
||
| fail("RSA " + keySize + " worked unexpectedly in FIPS mode"); | ||
| } catch (InvalidParameterException e) { | ||
| assertEquals("RSA keys must be at least 2048 bits long", e.getMessage()); |
There was a problem hiding this comment.
We should ensure that we get this error message only on RSA key sizes that are expected to fail.
if (keySize < 2048) {
assertEquals("RSA keys must be at least 2048 bits long", e.getMessage());
} else {
throw e;
}
| assertEquals("RSA keys must be at least 2048 bits long", e.getMessage()); | ||
| } | ||
|
|
||
| assumeTrue( |
There was a problem hiding this comment.
Seems like we already handle the expected behavior above in the try catch block ? Above we are asserting expected behavior and not sure why we turn around here and skip the test?
2 participants
This change consolidates various tests associated with the former BaseTestRSA into a single parameterized test.
Signed-off-by: Mohit Rajbhar mohit.rajbhar@ibm.com