GH-3652: Use assertThat checks in parquet-common tests#3654
Conversation
nastra
force-pushed
the
assertj-parquet-common
branch
from
3263eec to
03372b2
Compare
nastra
changed the title
| * @param expected An Exception class that the Runnable should throw | ||
| * @param callable A Callable that is expected to throw the exception | ||
| */ | ||
| public static void assertThrows(String message, Class<? extends Exception> expected, Callable callable) { |
There was a problem hiding this comment.
I've replaced all usages of these methods because using assertThatThrownBy() directly gives you greater flexibility in testing
| assertThat(new SemanticVersion(1, 8, 0)).isLessThan(new SemanticVersion(1, 8, 1)); | ||
| assertThat(new SemanticVersion(1, 8, 2)).isGreaterThan(new SemanticVersion(1, 8, 1)); | ||
|
|
||
| assertTrue(new SemanticVersion(1, 8, 1).compareTo(new SemanticVersion(1, 8, 1)) == 0); |
There was a problem hiding this comment.
these 3 checks were checking the same thing and were duplicates
|
|
||
| assertThrows( | ||
| "Should throw EOFException at end of stream", EOFException.class, (Callable<Integer>) stream::read); | ||
| assertThatThrownBy(stream::read).isInstanceOf(EOFException.class); |
There was a problem hiding this comment.
the exception message is null so we're omitting the hasMessage() check here and in a few other places. Depending on the JDK version, it's also possible that the exception either has a null message or has some message, so it's safer to omit the check
| assertThat(stream.read()).isEqualTo(i); | ||
| } | ||
|
|
||
| assertThrows( |
There was a problem hiding this comment.
we're replacing all usages of assertThrows because using assertThatThrownBy gives you greater flexibility for localized testing, in case a test case would want to add some additional checks on top of what already is being checked
| int i = 0; | ||
|
|
||
| ByteBuffer one = buffers.get(0); | ||
| Assert.assertSame("Should use the same backing array", one.array(), data.array()); |
There was a problem hiding this comment.
this is an example where the actual/expected parameters are flipped in the JUnit4 Assert. We're expecting one to be the actual object and compare it against data
nastra
force-pushed
the
assertj-parquet-common
branch
from
03372b2 to
6799b64
Compare
|
|
||
| // one is a view of the first buffer because it is smaller | ||
| ByteBuffer one = buffers.get(0); | ||
| Assert.assertSame( |
There was a problem hiding this comment.
actual/expected were flipped here as well
| assertThat(Strings.expandGlob("{}a")).containsExactly("a"); | ||
| assertThat(Strings.expandGlob("a{}")).containsExactly("a"); | ||
| assertThat(Strings.expandGlob("{,}")).containsExactly("", ""); | ||
| assertThat(Strings.expandGlob("a{b,{},c}")).containsExactly("ab", "a", "ac"); |
There was a problem hiding this comment.
we don't need to compare against actual lists. It's easier to just verify that the actual contains those elements in that specified order
| assertMatches(wp, "foo", "foo.x", "foo.x.y"); | ||
| assertDoesNotMatch(wp, "xfoo", "xfoox", "fooa.x.y"); | ||
|
|
||
| assertThat(wp.matches("foo")).isTrue(); |
There was a problem hiding this comment.
it is easier to debug a single line if the test ever fails
nastra
force-pushed
the
assertj-parquet-common
branch
from
6799b64 to
8741775
Compare
1 participant
Rationale for this change
Switching assertions from JUnit4-style Assert to AssertJ makes code easier to read and debug when tests fail.
What changes are included in this PR?
This updates parquet-common tests to use AssertJ checks, which provide more fluent assertions and more detailed information when assertions fails on CI.
The changes in this PR were done with the help of Claude but I manually reviewed every single LOC
Are these changes tested?
yes, existing tests
Are there any user-facing changes?
no