-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Create JUnit5 version of TableTestBase #9217
Conversation
.isEqualTo(expectedStatus); | ||
} | ||
|
||
Assertions.assertThat(expectedFiles.hasNext()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertions.assertThat(expectedFiles.hasNext()) | |
Assertions.assertThat(expectedFiles).isExhausted(); |
.isEqualTo(statuses.next()); | ||
} | ||
|
||
Assertions.assertThat(expectedFiles.hasNext()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertions.assertThat(expectedFiles.hasNext()) | |
Assertions.assertThat(expectedFiles).isExhausted(); |
} | ||
} | ||
|
||
Assertions.assertThat(expectedFiles.hasNext()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertions.assertThat(expectedFiles.hasNext()) | |
Assertions.assertThat(expectedFiles).isExhausted(); |
.isTrue(); | ||
} | ||
|
||
Assertions.assertThat(newManifests.size()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertions.assertThat(newManifests.size()) | |
Assertions.assertThat(newManifests).hasSize(1); |
7ca2786
to
d695238
Compare
@nastra Sorry to continue working so late. I got COVID last month and worked on my graduate school applications. |
public void testAppendCommitEvent() { | ||
Assert.assertEquals("Table should start empty", 0, listManifestFiles().size()); | ||
Assertions.assertThat(listManifestFiles()).as("Table should start empty").isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you statically import assertThat()
please?
"1", | ||
currentEvent.summary().get("total-data-files")); | ||
Assertions.assertThat(currentEvent).isNotNull(); | ||
Assertions.assertThat(currentEvent.summary().get("added-records")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be simplified to
assertThat(currentEvent.summary())
.containsEntry("added-records", "1")
.containsEntry("added-data-files", "1")
.containsEntry("total-records", "1")
.containsEntry("total-data-files", "1");
import org.junit.jupiter.api.TestTemplate; | ||
import org.junit.jupiter.api.extension.ExtendWith; | ||
|
||
@ExtendWith(ParameterizedTestExtension.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be part of the base class. Also the base + subclass shouldn't have a constructor anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that some classes, for example ScanPlanningAndReportingTestBase
and TestCommitReporting
only use value 2
for formatVersion(in their constructor), I'm not sure if it is ok for such classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are still parameterized test, but using only a single version in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I got it. I have tried to override the parameters()
with @Parameters
in subclass and it works.
You can push the changes to my branch. Thanks for pointing this out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll close this one.
@nastra Hi, as we talked in #9073, I create a copy of
TableTestBase
asTestBase
, which changed to Junit5+assertj style.Besides, I have tried the
ParameterizedTestExtension
in #9161, and it works well with the new veresionTestBase
's sub-class, which use test parameter in constructor.I will next continue work on issue #9085 which depends on this pr.