You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/04/18 23:25:23 UTC

[GitHub] [incubator-hudi] xushiyan opened a new pull request #1530: [HUDI-809] Migrate HoodieCommonTestHarness to JUnit 5

xushiyan opened a new pull request #1530: [HUDI-809] Migrate HoodieCommonTestHarness to JUnit 5
URL: https://github.com/apache/incubator-hudi/pull/1530
 
 
   * Create an interim test class `org.apache.hudi.common.testutils.HoodieCommonTestHarness` to migrate subclasses of `org.apache.hudi.common.HoodieCommonTestHarness`
   
   ### Migration status (after merging)
   
   | Package | JUnit 5 lib | API migration | Restructure packages |
   | --- | --- | --- | --- |
   | `hudi-cli` | βœ… | 🚧 | - |
   | `hudi-client` | βœ… | 🚧 | - |
   | `hudi-common` | βœ… | 🚧 | 🚧 |
   | `hudi-hadoop-mr` | βœ… | βœ… | - |
   | `hudi-hive-sync` | βœ… | βœ… | - |
   | `hudi-integ-test` | βœ… | βœ…  | N.A. |
   | `hudi-spark` | βœ… | βœ… | - |
   | `hudi-timeline-service` | βœ… | βœ… | - |
   | `hudi-utilities` | βœ… | 🚧 | - |
   
   
   
   ## Committer checklist
   
    - [ ] Has a corresponding JIRA in PR title & commit
    
    - [ ] Commit message is descriptive of the change
    
    - [ ] CI is green
   
    - [ ] Necessary doc changes done or have another open PR
          
    - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5
URL: https://github.com/apache/incubator-hudi/pull/1530#issuecomment-616025885
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=h1) Report
   > Merging [#1530](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/75523657a4b5c67209e48c1c989a4dd017b4fdd1&el=desc) will **increase** coverage by `0.01%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1530/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1530      +/-   ##
   ============================================
   + Coverage     72.28%   72.30%   +0.01%     
     Complexity      294      294              
   ============================================
     Files           374      374              
     Lines         16365    16365              
     Branches       1648     1648              
   ============================================
   + Hits          11830    11833       +3     
   + Misses         3804     3801       -3     
     Partials        731      731              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=tree) | Coverage Ξ” | Complexity Ξ” | |
   |---|---|---|---|
   | [...ache/hudi/common/fs/inline/InMemoryFileSystem.java](https://codecov.io/gh/apache/incubator-hudi/pull/1530/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2ZzL2lubGluZS9Jbk1lbW9yeUZpbGVTeXN0ZW0uamF2YQ==) | `89.65% <0.00%> (+10.34%)` | `0.00% <0.00%> (ΓΈ%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Ξ” = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=footer). Last update [7552365...2a3a0ff](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io commented on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5
URL: https://github.com/apache/incubator-hudi/pull/1530#issuecomment-616025885
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=h1) Report
   > Merging [#1530](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/75523657a4b5c67209e48c1c989a4dd017b4fdd1&el=desc) will **increase** coverage by `0.01%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1530/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1530      +/-   ##
   ============================================
   + Coverage     72.28%   72.30%   +0.01%     
     Complexity      294      294              
   ============================================
     Files           374      374              
     Lines         16365    16365              
     Branches       1648     1648              
   ============================================
   + Hits          11830    11833       +3     
   + Misses         3804     3801       -3     
     Partials        731      731              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=tree) | Coverage Ξ” | Complexity Ξ” | |
   |---|---|---|---|
   | [...ache/hudi/common/fs/inline/InMemoryFileSystem.java](https://codecov.io/gh/apache/incubator-hudi/pull/1530/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2ZzL2lubGluZS9Jbk1lbW9yeUZpbGVTeXN0ZW0uamF2YQ==) | `89.65% <0.00%> (+10.34%)` | `0.00% <0.00%> (ΓΈ%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Ξ” = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=footer). Last update [7552365...2a3a0ff](https://codecov.io/gh/apache/incubator-hudi/pull/1530?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#discussion_r412693956



##########
File path: hudi-common/src/test/java/org/apache/hudi/common/table/TestHoodieTableMetaClient.java
##########
@@ -18,41 +18,41 @@
 
 package org.apache.hudi.common.table;
 
-import org.apache.hudi.common.HoodieCommonTestHarness;
 import org.apache.hudi.common.model.HoodieTestUtils;
 import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
 import org.apache.hudi.common.table.timeline.HoodieInstant;
 import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.testutils.HoodieCommonTestHarnessJunit5;
 import org.apache.hudi.common.util.Option;
 
-import org.junit.Before;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
 import java.io.IOException;
 
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 
 /**
  * Tests hoodie table meta client {@link HoodieTableMetaClient}.
  */
-public class TestHoodieTableMetaClient extends HoodieCommonTestHarness {
+public class TestHoodieTableMetaClient extends HoodieCommonTestHarnessJunit5 {
 
-  @Before
+  @BeforeEach
   public void init() throws IOException {
     initMetaClient();
   }
 
   @Test
   public void checkMetadata() {
-    assertEquals("Table name should be raw_trips", HoodieTestUtils.RAW_TRIPS_TEST_NAME,
-        metaClient.getTableConfig().getTableName());
-    assertEquals("Basepath should be the one assigned", basePath, metaClient.getBasePath());
-    assertEquals("Metapath should be ${basepath}/.hoodie", basePath + "/.hoodie", metaClient.getMetaPath());
+    assertEquals(HoodieTestUtils.RAW_TRIPS_TEST_NAME, metaClient.getTableConfig().getTableName(), "Table name should be raw_trips");

Review comment:
       here

##########
File path: hudi-common/src/test/java/org/apache/hudi/common/table/TestHoodieTableMetaClient.java
##########
@@ -67,16 +67,15 @@ public void checkSerDe() {
     commitTimeline.saveAsComplete(instant, Option.of("test-detail".getBytes()));
     commitTimeline = commitTimeline.reload();
     HoodieInstant completedInstant = HoodieTimeline.getCompletedInstant(instant);
-    assertEquals("Commit should be 1 and completed", completedInstant, commitTimeline.getInstants().findFirst().get());
-    assertArrayEquals("Commit value should be \"test-detail\"", "test-detail".getBytes(),
-        commitTimeline.getInstantDetails(completedInstant).get());
+    assertEquals(completedInstant, commitTimeline.getInstants().findFirst().get(), "Commit should be 1 and completed");
+    assertArrayEquals("test-detail".getBytes(), commitTimeline.getInstantDetails(completedInstant).get(), "Commit value should be \"test-detail\"");

Review comment:
       here

##########
File path: hudi-common/src/test/java/org/apache/hudi/common/table/TestHoodieTableMetaClient.java
##########
@@ -85,21 +84,20 @@ public void checkCommitTimeline() {
     // Commit timeline should not auto-reload every time getActiveCommitTimeline(), it should be cached
     activeTimeline = metaClient.getActiveTimeline();
     activeCommitTimeline = activeTimeline.getCommitTimeline();
-    assertTrue("Should be empty commit timeline", activeCommitTimeline.empty());
+    assertTrue(activeCommitTimeline.empty(), "Should be empty commit timeline");
 
     HoodieInstant completedInstant = HoodieTimeline.getCompletedInstant(instant);
     activeTimeline = activeTimeline.reload();
     activeCommitTimeline = activeTimeline.getCommitTimeline();
-    assertFalse("Should be the 1 commit we made", activeCommitTimeline.empty());
-    assertEquals("Commit should be 1", completedInstant, activeCommitTimeline.getInstants().findFirst().get());
-    assertArrayEquals("Commit value should be \"test-detail\"", "test-detail".getBytes(),
-        activeCommitTimeline.getInstantDetails(completedInstant).get());
+    assertFalse(activeCommitTimeline.empty(), "Should be the 1 commit we made");
+    assertEquals(completedInstant, activeCommitTimeline.getInstants().findFirst().get(), "Commit should be 1");
+    assertArrayEquals("test-detail".getBytes(), activeCommitTimeline.getInstantDetails(completedInstant).get(), "Commit value should be \"test-detail\"");

Review comment:
       here




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-hudi] xushiyan commented on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
xushiyan commented on issue #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#issuecomment-616297099


   @yanghua Thank you for taking care of the review. To better coordinate, just want to clarify the steps I'm taking for migrating the APIs:
   
   1. (This PR) Introduce `org.apache.hudi.common.testutils.HoodieCommonTestHarness` to allow incrementally migrate its subclasses, and complete migrations for `hudi-hadoop-mr`
   2. Migrate all subclasses of `HoodieClientTestHarness` (this completes `hudi-cli`)
   3. Migrate the remaining `HoodieCommonTestHarness` subclasses (this will merge `org.apache.hudi.common.HoodieCommonTestHarness` to the interim one in 1) )
   4. Migrate the remaining tests in `hudi-client`, `hudi-utilities`, and `hudi-common` 
   
   I'll try to keep the PR diff less than 500 +/- to make the review less daunting..but in case of 2) where all subclasses need to go at once, the diff may go up for that PR... just as a heads-up. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#discussion_r412346677



##########
File path: hudi-common/src/test/java/org/apache/hudi/common/table/view/TestHoodieTableFileSystemView.java
##########
@@ -59,9 +58,10 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+

Review comment:
       removed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-hudi] yanghua commented on a change in pull request #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#discussion_r411988213



##########
File path: hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieCommonTestHarness.java
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.testutils;
+
+import org.apache.hudi.common.model.HoodieTestUtils;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.IOException;
+
+/**
+ * The JUnit 5 version of {@link org.apache.hudi.common.HoodieCommonTestHarness}.
+ * <p>
+ * To incrementally migrate test classes.
+ */
+public class HoodieCommonTestHarness extends org.apache.hudi.common.HoodieCommonTestHarness {

Review comment:
       Can we use another name to distinguish each other?

##########
File path: hudi-common/src/test/java/org/apache/hudi/common/table/view/TestHoodieTableFileSystemView.java
##########
@@ -59,9 +58,10 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+

Review comment:
       redundant empty line?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-hudi] xushiyan commented on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
xushiyan commented on issue #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#issuecomment-617580627


   @yanghua noted. Will revert the styles back in the next migration PR. thanks.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-hudi] xushiyan commented on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
xushiyan commented on issue #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#issuecomment-617422244


   @yanghua Please kindly check the last 2 commits for the latest changes.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-hudi] xushiyan commented on a change in pull request #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
xushiyan commented on a change in pull request #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#discussion_r412357516



##########
File path: hudi-common/src/test/java/org/apache/hudi/common/testutils/HoodieCommonTestHarness.java
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.testutils;
+
+import org.apache.hudi.common.model.HoodieTestUtils;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.IOException;
+
+/**
+ * The JUnit 5 version of {@link org.apache.hudi.common.HoodieCommonTestHarness}.
+ * <p>
+ * To incrementally migrate test classes.
+ */
+public class HoodieCommonTestHarness extends org.apache.hudi.common.HoodieCommonTestHarness {

Review comment:
       Fixed. Just note that the name should be renamed back to `HoodieCommonTestHarness ` once migrations done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-hudi] yanghua commented on issue #1530: [HUDI-809] Migrate CommonTestHarness to JUnit 5

Posted by GitBox <gi...@apache.org>.
yanghua commented on issue #1530:
URL: https://github.com/apache/incubator-hudi/pull/1530#issuecomment-616302909


   > @yanghua Thank you for taking care of the review. To better coordinate, just want to clarify the steps I'm taking for migrating the APIs:
   > 
   > 1. (This PR) Introduce `org.apache.hudi.common.testutils.HoodieCommonTestHarness` to allow incrementally migrate its subclasses, and complete migrations for `hudi-hadoop-mr`
   > 2. Migrate all subclasses of `HoodieClientTestHarness` (this completes `hudi-cli`)
   > 3. Migrate the remaining `HoodieCommonTestHarness` subclasses (this will merge `org.apache.hudi.common.HoodieCommonTestHarness` to the interim one in 1) )
   > 4. Migrate the remaining tests in `hudi-client`, `hudi-utilities`, and `hudi-common`
   > 
   > I'll try to keep the PR diff less than 500 +/- to make the review less daunting..but in case of 2) where all subclasses need to go at once, the diff may go up for that PR... just as a heads-up.
   
   Sounds reasonable.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org