Skip to content

HADOOP-14693. Test PR: Ran rewrite plugin on hadoop-hdfs module to upgrade to JUnit 5 #3304

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

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

aajisaka
Copy link
Member

@aajisaka aajisaka commented Aug 14, 2021

Description of PR

Test rewrite-maven-plugin https://docs.openrewrite.org/tutorials/migrate-from-junit-4-to-junit-5

How was this patch tested?

Not tested

For code changes:

  • Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
  • Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

@hadoop-yetus

This comment has been minimized.

@aajisaka
Copy link
Member Author

The plugin failed to convert some contract tests.

@smengcl
Copy link
Contributor

smengcl commented Aug 20, 2021

Thanks @aajisaka for trying out the rewrite plugin.

Overall the tool seems to be doing a pretty good job.

  1. In the case of TestHDFSContractPathHandle.java, It somehow attempts to import org.junit.jupiter.api.Assertions.super and rearrange the argument order to super() call, which is kinda amusing.
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/contract/hdfs/TestHDFSContractPathHandle.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/contract/hdfs/TestHDFSContractPathHandle.java
index c65a60b18b1..a5ea66782cf 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/contract/hdfs/TestHDFSContractPathHandle.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/contract/hdfs/TestHDFSContractPathHandle.java
@@ -21,11 +21,13 @@
...
+import static org.junit.jupiter.api.Assertions.super;
+
 /**
  * Verify HDFS compliance with {@link org.apache.hadoop.fs.PathHandle}
  * semantics.
@@ -35,15 +37,15 @@
 
   public TestHDFSContractPathHandle(String testname, Options.HandleOpt[] opts,
       boolean serialized) {
-    super(testname, opts, serialized);
+      super(opts, serialized, testname);
   }
...
  1. In the case of TestDiskBalancerRPC.java, it does remove JUnit 4's ExpectedException variables, but it doesn't seem to rewrite the ExpectedException.expect usages.

For instance,

  @Rule
  public ExpectedException thrown = ExpectedException.none();
...
  @Test
  public void testSubmitPlanWithInvalidVersion() throws Exception {
    ...
    thrown.expect(DiskBalancerException.class);
    thrown.expect(new DiskBalancerResultVerifier(Result.INVALID_PLAN_VERSION));
    dataNode.submitDiskBalancerPlan(planHash, planVersion, PLAN_FILE,
        plan.toJson(), false);

should have been rewritten into something like:

    final DiskBalancerException thrown =
        Assertions.assertThrows(DiskBalancerException.class, () -> {
          dataNode.submitDiskBalancerPlan(planHash, planVersion, PLAN_FILE,
              plan.toJson(), false);
        });
    Assertions.assertEquals(thrown.getResult(), Result.INVALID_PLAN_VERSION);

... and some other interesting changes by rewrite plugin.

I have included a bunch of fixes in my fork of your branch that addresses all these rewrite errors I mentioned above and a few others here:

https://github.com/apache/hadoop/compare/aajisaka:rewrite-junit5-hdfs..smengcl:rewrite-junit5-hdfs?diff=split

Compliation passed locally. I think I might file a PR to trigger the CI later.

@smengcl
Copy link
Contributor

smengcl commented Aug 20, 2021

Filed #3316 for CI on my branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants