Bugfix: pemEncoded must not produce empty lines #31
Conversation
| @@ -157,6 +157,24 @@ | |||
| "lodash.get": "^4.4.2" | |||
| } | |||
| }, | |||
| "@types/color-name": { | |||
There was a problem hiding this comment.
Please revert the lockfile changes.
| mockPemEncodedCert = require('./mocks/mock.pem'), | ||
| mockBase64EncodedRawBuffer = require('./mocks/base64buffer.txt'), | ||
| getSSLCertificate = require('../index'); | ||
| getSSLCertificate = rewire('../index'); |
I'm unsure if this is really needed. Is it because the method is private? |
|
I think the PR would be good for https://github.com/DanielRuf/get-ssl-certificate-next - can you provide it there too (without the lockfile changes)? Regarding rewire, wouldn't it be better if we also export the pemEncode method for users or should we keep the method private and switch to rewire? |
Yes, I found rewire as a solution to test private methods. The lockfile changes come from rewire. Exporting this tiny helper just for testing feels wrong as you would end up exporting all internal functions. Another solution could be to extract pemEncode to a separate file, add tests to it and import it. For a change like this I am a little short of time. Another obvious workaround would be to just apply the fix " && i < str.length" and leave out the new tests. There were no tests present in the first place. |
In my fork (get-ssl-certificate-next) I update the lockfile due to security reasons. And when lockfiles are changed in multiple PRs and one is merged or the file is updated on the master branch all PRs will get a conflict. |
|
Tests are always good and I think so far the reasoning for rewire is ok so this could stay like this - personally I just prefer less dependencies and completely exported objects with all methods. |
As described in #30 the encoded pem could contain an empty line at the end of the payload which rendered the data invalid. This is fixed and tests regarding the function pemEncode werde added.
In order to test the internal pemEncode function "rewire" was used as a replacement for require.