Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

lisirrx
Copy link
Contributor

@lisirrx lisirrx commented Dec 5, 2023

@nastra Hi, as we talked in #9073, I create a copy of TableTestBase as TestBase, which changed to Junit5+assertj style.
Besides, I have tried the ParameterizedTestExtension in #9161, and it works well with the new veresion TestBase's sub-class, which use test parameter in constructor.
I will next continue work on issue #9085 which depends on this pr.

@github-actions github-actions bot added the core label Dec 5, 2023
.isEqualTo(expectedStatus);
}

Assertions.assertThat(expectedFiles.hasNext())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assertions.assertThat(expectedFiles.hasNext())
Assertions.assertThat(expectedFiles).isExhausted();

.isEqualTo(statuses.next());
}

Assertions.assertThat(expectedFiles.hasNext())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assertions.assertThat(expectedFiles.hasNext())
Assertions.assertThat(expectedFiles).isExhausted();

}
}

Assertions.assertThat(expectedFiles.hasNext())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assertions.assertThat(expectedFiles.hasNext())
Assertions.assertThat(expectedFiles).isExhausted();

.isTrue();
}

Assertions.assertThat(newManifests.size())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assertions.assertThat(newManifests.size())
Assertions.assertThat(newManifests).hasSize(1);

@nastra nastra changed the title feat: add a Junit5 version of TableTestBase Core: Create JUnit5 version of TableTestBase Dec 5, 2023
@lisirrx
Copy link
Contributor Author

lisirrx commented Jan 4, 2024

@nastra Sorry to continue working so late. I got COVID last month and worked on my graduate school applications.
I add the TestBase class and give 2 examples using it.

public void testAppendCommitEvent() {
Assert.assertEquals("Table should start empty", 0, listManifestFiles().size());
Assertions.assertThat(listManifestFiles()).as("Table should start empty").isEmpty();
Copy link
Contributor

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"))
Copy link
Contributor

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)
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lisirrx I've opened #9424 to fix the parameterization as that should live in the base class. If you're ok with the changes I can push them to your branch

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastra I noticed you have merged #9424, so should this pr be closed?
BTW, Should #9085 be split into multiple tasks? I want to work on it but there are too many tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lisirrx yes this PR can be closed and we can just open multiple PRs referencing #9085 (without having to create subtasks)

Copy link
Contributor Author

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.

@Fokko Fokko removed the Specification Issues that may introduce spec changes. label Jan 8, 2024
@lisirrx lisirrx closed this Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants