Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reintroduce and fix test for Pkcs12StoreBuilder #595

Closed
wants to merge 1 commit into from

Conversation

bb-froggy
Copy link

Describe your changes

I found the methods DoTestSupportedTypes and basicStoreTest in Pkcs12StoreTest, which performed some IMHO useful tests on Pkcs12Store, but weren't actually part of any test (and therefore never called).

I added some code to actually call them. For this, I refactored some code into the new method CreateTestCertificateAndKey from another test to accomplish the goal with the minimum of changes possible. The other tests do not behave different now, except for one constructor call which explicitly passes a parameter that was previously not passed and the default.

I had to explicitly add the Pkcs9LocalKeyId to the X509CertificateEntry, otherwise the test failed, because the PrivateKeyEntry will always have a Pkcs9LocalKeyId added by the Pkcs12Store. I found this confusing, but maybe I am missing some detail. I consider this issue outside the scope of this change, though.

Sorry for the whitespace/tab changes that might make this change more difficult to read... the code file had a mix of tab and space indentations to begin with, so there was no consistent way to make these modifications. I could make the file use a single type of indentation in a separate patch, if you want, to make this easier to read. I only need to know whether you prefer the tabs or how many spaces?

How has this been tested?

I ran the modified tests and they pass.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have kept the patch limited to only change the parts related to the patch
  • This change requires a documentation update

See also Contributing Guidelines.

@peterdettman
Copy link
Collaborator

Merged as part of #596 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants