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

Dell : Migrate Files using TestRule to Junit5. #8707

Merged
merged 9 commits into from
Oct 10, 2023

Conversation

ashutosh-roy
Copy link
Contributor

Description

Closes #7888.

This PR updates some of the test files in dell package which uses @Rule, @ClassRule or extend TestRule to Junit5.

import org.junit.runners.model.Statement;
import org.junit.jupiter.api.extension.AfterEachCallback;
import org.junit.jupiter.api.extension.BeforeEachCallback;
import org.junit.jupiter.api.extension.ExtensionContext;

/**
* Mock rule of ECS S3 mock.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we update the javadoc too?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on updating javadoc

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 am thinking of adding these annotations in this section ? Does that help or there's some other place I need to update in?

Copy link
Contributor

Choose a reason for hiding this comment

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

as part of this PR I think what makes sense is to change the javadoc here from Mock rule of ECS S3 mock to Test extension for using an ECS S3 mock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also update the class name from EcsS3MockRule to EcsS3MockExtension at all the places for better readability.


@Override
public void beforeEach(ExtensionContext extensionContext) {
System.out.println("Bucket removed");
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems here we are trying to initialise the bucket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nk1506 if there are no feedbacks, are we good to merge this?

@ashutosh-roy
Copy link
Contributor Author

Hi @nastra, Please provide your feedbacks on the PR for this issue.

build.gradle Outdated
@@ -912,7 +912,6 @@ project(':iceberg-dell') {
implementation project(':iceberg-common')
implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow')
compileOnly libs.object.client.bundle

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this ✅


@Override
public void afterEach(ExtensionContext extensionContext) {
System.out.println("Bucket removed");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove those println statements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed ✅

build.gradle Outdated
@@ -987,5 +987,4 @@ String getJavadocVersion() {
apply from: 'jmh.gradle'
apply from: 'baseline.gradle'
apply from: 'deploy.gradle'
apply from: 'tasks.gradle'

apply from: 'tasks.gradle'
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary change

build.gradle Outdated
@@ -988,4 +988,3 @@ apply from: 'jmh.gradle'
apply from: 'baseline.gradle'
apply from: 'deploy.gradle'
apply from: 'tasks.gradle'

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this still has a diff

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM once Javadoc is updated and the change on build.gradle is reverted

@nastra nastra merged commit 90cf38c into apache:master Oct 10, 2023
45 checks passed
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.

Migrate Files using TestRule in dell package to Junit5
3 participants