search

shred / acme4j

Java client for ACME (Let's Encrypt)
https://acme4j.shredzone.org
Apache License 2.0
520 stars 96 forks source link

added method to allow the setting of attributes of the CSR's DN #129

Closed kimmerin closed 2 years ago

kimmerin commented 2 years ago

Hi,

as discussed in the issue here is the PR containing my change and the corresponding test case. I allowed myself to do some refactoring in the test case to avoid code duplication as well.

shred commented 2 years ago

Thank you for your PR! I also appreciate that you cleaned up the unit test. :smile:

The main change looks good to me!

For the unit test, acme4j has recently switched to the AssertJ framework. This means that instead of assertEquals(message, value, expected), the more speaking pattern assertThat(value).as(message).isEqualTo(expected) should be used.

For code blocks like:

IllegalStateException ise;
ise = assertThrows(IllegalStateException.class, builder::getCSR, "getCSR()");
Assert.assertEquals("unexpected exception message", "sign CSR first", ise.getMessage());

AssertJ shows its strength with patterns like:

assertThatExceptionOfType(IllegalStateException.class)
    .isThrownBy(builder::getCSR)
    .as("getCSR()")
    .withMessage("sign CSR first");

For IllegalStateExceptions you could even use assertThatIllegalStateException(). Can you change your tests?

shred commented 2 years ago

Looks good now! Thank you for this PR!

kimmerin commented 2 years ago

You're welcome. Reiner Eigennutz ;-)