GH-3615: Use assertThat checks in parquet-variant tests#3617
Conversation
| } | ||
| // An object header | ||
| Variant value = new Variant(ByteBuffer.wrap(new byte[] {0b1000010}), VariantTestUtil.EMPTY_METADATA); | ||
| assertThatThrownBy(value::numArrayElements) |
There was a problem hiding this comment.
Previously with Assert.fail we would have a larger block where it wasn't clear code line of code would actually throw the exception and what error msg would be returned. We're more explicit now in terms of testing error conditions, because we check the underlying exception and the error msg.
| "Expected decimal type, got " + type, | ||
| type == Variant.Type.DECIMAL4 || type == Variant.Type.DECIMAL8 || type == Variant.Type.DECIMAL16); | ||
| Assert.assertEquals(0, new BigDecimal("3.14").compareTo(v.getDecimal())); | ||
| assertThat(type).isIn(Variant.Type.DECIMAL4, Variant.Type.DECIMAL8, Variant.Type.DECIMAL16); |
There was a problem hiding this comment.
if this check ever fails, then it will me much clearer what the issue is, because previously it would only say false is not true. I've modified the check to show how such an error msg would look:
Expecting actual:
DECIMAL16
to be in:
[DECIMAL4, DECIMAL8]
| .hasMessageContaining("was expecting double-quote to start field name"); | ||
| } | ||
|
|
||
| @Test(expected = IOException.class) |
There was a problem hiding this comment.
we're also more explicit here in terms of testing failure conditions
nastra
force-pushed
the
assertj-parquet-variant
branch
from
e65071b to
7215ca4
Compare
|
@nastra I think there are several libraries that provide this functionality (e.g. I think there is a Google Truth library) maybe start a thread on the mailing list to just make sure nobody has any strong technical reasons for preferring one over the other. |
2 participants
Rationale for this change
This introduces AssertJ for testing, which enables more fluent assertion checks, easier debugging and overall easier testing. Additional details about the motivation can be found in #3615
What changes are included in this PR?
added the assertj library to the test scope. Also updated parquet-variant to use the new
assertThatchecks to make test code more readableAre these changes tested?
yes, existing tests
Are there any user-facing changes?
no