Refactor JUnit 5 Tests for BaseTestDSASignatureInterop#1468
Refactor JUnit 5 Tests for BaseTestDSASignatureInterop#1468Mohit-Rajbhar100698 wants to merge 1 commit into
Conversation
Mohit-Rajbhar100698
force-pushed
the
refactor-junit5-BaseTestDSASignatureInterop-tests
branch
from
90700db to
edfcd4e
Compare
Mohit-Rajbhar100698
requested review from
KostasTsiounis,
jasonkatonica and
johnpeck-us-ibm
| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| setAndInsertProvider(provider); | ||
| setInteropProviderName(TestProvider.SUN.getProviderName()); |
There was a problem hiding this comment.
Should we hardcode this here or have the interop provider as a parameter too? @jasonkatonica what do you think?
There was a problem hiding this comment.
I’m only using enum TestProvider.SUN here to get the provider name for the interop provider .
not passing through the JUnit 5 parameterization.
There was a problem hiding this comment.
For consistancy i think i woudl prefer using parameters like other interop tests. Sure in this case we are only doing it against SUN however in the future it would be easier to add other providers using same code pattern as other interop tests.
There was a problem hiding this comment.
Additionally we could remove SUN from the testcase name to generalize it as a interop test for DSA signatures.
There was a problem hiding this comment.
Also upon further inspection there is a test TestDSASignatureInteropBC I think this test should also be included in this update ( and refactored to use parameters and the BC provider )
There was a problem hiding this comment.
For consistancy i think i woudl prefer using parameters like other interop tests. Sure in this case we are only doing it against SUN however in the future it would be easier to add other providers using same code pattern as other interop tests.
I have parameterized the interop provider as well.
There was a problem hiding this comment.
Additionally we could remove SUN from the testcase name to generalize it as a interop test for DSA signatures.
Removed SUN from test name
There was a problem hiding this comment.
Also upon further inspection there is a test
TestDSASignatureInteropBCI think this test should also be included in this update ( and refactored to use parameters and the BC provider )
Since TestDSASignatureInteropBC extends BaseTestDSASignatureInterop2, As we discussed we will handle that refactoring separately in another PR.
Mohit-Rajbhar100698
force-pushed
the
refactor-junit5-BaseTestDSASignatureInterop-tests
branch
from
edfcd4e to
893ded6
Compare
Replace multiple test classes extending BaseTestDSASignatureInterop with a single JUnit 5 parameterized test class Signed-off-by: Mohit Rajbhar <mohit.rajbhar@ibm.com>
Mohit-Rajbhar100698
force-pushed
the
refactor-junit5-BaseTestDSASignatureInterop-tests
branch
from
893ded6 to
d9a6071
Compare
| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| setAndInsertProvider(provider); | ||
| setInteropProviderName(interopProvider.getProviderName()); |
There was a problem hiding this comment.
Would it make sense to do a setAndInsertProvider() for the interop provider too, in case it's not in the provider list? I know we are just using the Sun provider here, but I think it's good practice.
4 participants
Replace multiple test classes extending BaseTestDSASignatureInterop with a single JUnit 5 parameterized test class
Signed-off-by: Mohit Rajbhar mohit.rajbhar@ibm.com