assertj icon indicating copy to clipboard operation
assertj copied to clipboard

Add `bytes()`/`bytes(Charset)`/`bytes(String)` navigation methods to `AbstractStringAssert`

Open onacit opened this issue 2 years ago • 9 comments

Feature summary

Add AbstractStirngAssert#extracting(Charset)AbstractByteArrayAssert so that the byte[] can be verified.

Example

String actual  = "";
assertThat(actual)
        .hasSize(0)
        .extracting(StandardCharset.US_ASCII) // AbstractByteArrayAssert
        .hasSize(0)
;

onacit avatar Oct 20 '23 18:10 onacit

What if we would stay closer to getBytes(Charset)?

Following the style of other navigation methods, we could add bytes(Charset):

String actual  = "";
assertThat(actual)
        .hasSize(0)
        .bytes(StandardCharset.US_ASCII) // AbstractByteArrayAssert
        .hasSize(0)

I would also consider the variant without parameters and the one with String.

scordio avatar Oct 20 '23 20:10 scordio

@scordio The bytes(Charset) seems nice. Thank you.

onacit avatar Oct 22 '23 08:10 onacit

@scordio I was looking into this one as it seems like a good starting point for me to contribute.

I've noticed that there are already methods such as AbstractStringAssert::asByte and AbstractByteArrayAssert::asString and AbstractByteArrayAssert::asString(Charset). What do you think about using the "as" prefix for consistency? so basically asBytes() , asBytes(Charset) and asBytes(String)

Lastly, what do you think the expected behavior should be in case of a null (actual) String? Should It throw an exception right away? or return a ByteArrayAssert with a null actual value? There are a few examples in the PR link

assertThat((String) null).asBytes() // throws error right away?
// or allows
assertThat((String) null).asBytes().isNull() 

etrandafir93 avatar Oct 23 '23 11:10 etrandafir93

Thanks for your interest, @etrandafir93!

I've noticed that there are already methods such as AbstractStringAssert::asByte and AbstractByteArrayAssert::asString and AbstractByteArrayAssert::asString(Charset). What do you think about using the "as" prefix for consistency? so basically asBytes() , asBytes(Charset) and asBytes(String)

To the best of my knowledge, there is no asByte in AbstractStringAssert but we do have other methods like asBase64Decoded.

We tend to favor the as prefix for methods that perform additional transformations under the hood, while we prefer not to use any prefix when we delegate the execution to an API provided by the underlying type. An example is AbstractThrowableAssert::cause that delegates to Throwable::getCause.

For this reason, I would go with bytes for this case.

Lastly, what do you think the expected behavior should be in case of a null (actual) String? Should It throw an exception right away? or return a ByteArrayAssert with a null actual value? There are a few examples in the PR link

It doesn't make much sense to execute navigation methods when the object under test is null, therefore I suggest performing an isNotNull() call internally before creating the new assertion object.

scordio avatar Oct 23 '23 20:10 scordio

@scordio thanks for the quick feedback! This is the asByte() method i was referring to. I have now applied the suggestions and fixed the formatting issues.

etrandafir93 avatar Oct 23 '23 22:10 etrandafir93

Right, I was mistakenly looking at the released version instead of the latest codebase, sorry 🙂

scordio avatar Oct 23 '23 23:10 scordio

Hello @scordio, @joel-costigliola,

I've opened the https://github.com/assertj/assertj/pull/3232 some time ago. I'd appreciate your feedback here whenever you have a moment - your time and expertise in reviewing this would mean a lot.

Let me know if there's anything I can do to assist in the process. Thanks, and I'm looking forward to your response.

etrandafir93 avatar Nov 17 '23 19:11 etrandafir93

Hi @etrandafir93 , thanks a lot and apologies for the delay.

We're currently finalizing 3.25.0 and we'll need some time before we can jump on your PR.

As we want to freeze the release scope, I'll schedule your changes for 3.26.0.

scordio avatar Nov 18 '23 09:11 scordio

@scordio no worries - thanks for reaching out and explaining the process!

etrandafir93 avatar Nov 18 '23 14:11 etrandafir93

Closed by https://github.com/assertj/assertj/pull/3232

joel-costigliola avatar May 12 '24 15:05 joel-costigliola