DRILL-8117: Upgrade unit tests to the cluster fixture framework #2499

kingswanwho · 2022-03-20T05:26:42Z

DRILL-8117: DRILL-8117: Upgrade unit tests to the cluster fixture framework

Description

Upgrades various unit tests to the cluster fixture framework and replaces other instances of deprecated code usage.

Methods client.alterSession() and client.alterSystem() run sql silently which would not throw exception, so it doesn't match the purpose for some test cases. And I still keep using run() here for consistence.

For TestInboundImpersonation
Because cluster fixture save client properties even client closed, use a dedicated cluster fixture for each test case to ensure the client fixture has clean properties.

For TestImpersonationMetadata
Only use one cluster from test start to the end will induce buffer overflow. So change BeforeClass to Before to start a new cluster for each test case.

Documentation

N/A

Testing

Existing unit tests.

paul-rogers · 2022-03-23T06:49:50Z

exec/java-exec/src/test/java/org/apache/drill/exec/impersonation/TestInboundImpersonation.java

@@ -156,22 +159,25 @@ public void unauthorizedTarget() throws Exception {

  @Test
  public void invalidPolicy() throws Exception {
-    thrownException.expect(new UserExceptionMatcher(UserBitShared.DrillPBError.ErrorType.VALIDATION,
-        "Invalid impersonation policies."));
+    String query = "ALTER SYSTEM SET `%s`='%s'";


We have methods that do this work: it is very common. Maybe the required clients are not available in this particular test?

Hi Paul,
Greetings to you!
I did this because the UT code in Drill used ExpectedException thrown = ExpectedException.none();
for ExpectedException instantiation.
However the method ExpectedException.none() has been deprecated, so I replaced ExpectedException.none() by Assert.assertThrows as the deprecation notice suggested.
Could you please give some suggestions will be really appreciated.
Best,
Jingchuan

Just picking one test file at random, here's an example of the idea: https://github.com/apache/drill/blob/master/exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestLargeFileCompilation.java#L273

It is not about the exception handling, which is fine. It's just about the busy-work of formatting the ALTER SYSTEM SET commands over and over.

Again, without looking closely, I don't know if this test happens to use one of the two clients that have these methods. If not, then you have to do things the tedious way.

Hi Paul, it does really help! I will replace all those hard-code in my PR by client thanks!

@paul-rogers Hi Paul, I am sorry that I dragged this PR out too long. I replaced hard-code to FixtureClient, but UT failed due to the Error:
java.lang.AssertionError: unexpected exception type thrown; expected:<org.apache.drill.common.exceptions.UserException> but was:<java.lang.IllegalStateException> at org.apache.drill.exec.impersonation.TestInboundImpersonation.invalidProxy(TestInboundImpersonation.java:186) Caused by: java.lang.IllegalStateException: org.apache.drill.common.exceptions.UserRemoteException: PERMISSION ERROR: Cannot change option exec.impersonation.inbound_policies in scope SESSION
I think the two clients cannot exist at the same time, so I changed back to the hard-code way.

cgivre · 2022-07-08T12:35:16Z

Hi @kingswanwho @paul-rogers We're getting ready to release Drill 1.20.2 which is a minor bugfix release. Is this PR ready to be merged and if so should it be included?

jnturton · 2022-07-11T08:55:40Z

@cgivre, looking over this PR I think it falls closer to cleanup than it does to bugfix. While it no doubt carries only a little regression risk, it also also offers no benefits to stable release users - it's we Drill devs that will benefit from it - so my vote here is that it is not backported and is merged only into master...

cgivre · 2022-07-19T00:42:57Z

@cgivre, looking over this PR I think it falls closer to cleanup than it does to bugfix. While it no doubt carries only a little regression risk, it also also offers no benefits to stable release users - it's we Drill devs that will benefit from it - so my vote here is that it is not backported and is merged only into master...

@jnturton I'm fine with that. I guess my question is can we merge it?

jnturton · 2022-09-08T09:08:49Z

Hi @kingswanwho everything here looks good to me, let's just see if we can replace the ALTER SYSTEMs with client.alterSystems.

jnturton · 2022-09-08T09:06:19Z

exec/java-exec/src/test/java/org/apache/drill/exec/impersonation/TestInboundImpersonation.java

@@ -156,22 +159,25 @@ public void unauthorizedTarget() throws Exception {

  @Test
  public void invalidPolicy() throws Exception {
-    thrownException.expect(new UserExceptionMatcher(UserBitShared.DrillPBError.ErrorType.VALIDATION,
-        "Invalid impersonation policies."));
+    String query = "ALTER SYSTEM SET `%s`='%s'";


Did you try the following here?

client.alterSystem(...); try { // run test } finally { client.resetSystem(...); }

@jnturton Hi James, the client here inherits from BaseTestQuery, which is DrillClient but not ClientFixture in TestLargeFileCompilation as Paul suggested. DrillClient doesn't have method like client.alterSystem(). BaseTestQuery is deprecated actually, but not marked as deprecated because it is still widely used. The best choice here I think is change base test class from BaseTestQuery to ClusterTest so that we could use client.alterSystem() for clean and compact code style. There are multiple Inheritance levels here, which are TestInboundImpersonation -> BaseTestImpersonation -> PlanTestBase -> BaseTestQuery. Due to PlanTestBase involves many classes. Maybe change BaseTestImpersonation inheritance from PlanTestBase to ClusterTest directly is an easier way. However, this PR mainly focus on the deprecated clean up, but induces more modifications on UT. Is this an appropriate change here?

Ah, okay. In that case I think we can go two ways here, since upgrading this test, or family of tests, to ClusterTest could be a significant expansion of PR's scope.

We can merge this as it is and leave the ClusterTest work for another PR or

we can expand the scope here and you can convert BaseTestImpersonation from PlanTestBase to ClusterTest. I believe that ClusterTest offers the plan string matching features in PlanTestBase.

I leave the choice for you to make based on which way you prefer to work.

Hi James, I would like to apply the second way, since this PR has been existing for a while, it doesn't matter to wait for a bit longer : )

Okay great! Let's switch this PR to Draft mode to show that you're still busy.

jnturton · 2023-02-01T14:09:10Z

exec/java-exec/src/test/java/org/apache/drill/test/ClusterTest.java

+
+  /**
+   * Utility method which tests given query produces a {@link UserException} and the exception message contains
+   * the given message.
+   * @param testSqlQuery Test query
+   * @param expectedErrorMsg Expected error message.
+   */
+  protected static void errorMsgTestHelper(String testSqlQuery, String expectedErrorMsg) throws Exception {
+    try {
+      run(testSqlQuery);
+      fail("Expected a UserException when running " + testSqlQuery);
+    } catch (UserException actualException) {
+      try {
+        assertThat("message of UserException when running " + testSqlQuery, actualException.getMessage(), containsString(expectedErrorMsg));
+      } catch (AssertionError e) {
+        e.addSuppressed(actualException);
+        throw e;
+      }
+    }
+  }
+
+  protected static void updateClient(Properties properties) {
+    if (client != null) {
+      client.close();
+      client = null;
+    }
+    ClientFixture.ClientBuilder clientBuilder = cluster.clientBuilder();
+    if (properties != null) {
+      for (final String key : properties.stringPropertyNames()) {
+        final String lowerCaseKey = key.toLowerCase();
+        clientBuilder.property(lowerCaseKey, properties.getProperty(key));
+      }
+    }
+    client = clientBuilder.build();
+  }
+
+  protected static void updateClient(final String user) {
+    updateClient(user, null);
+  }
+
+  protected static void updateClient(final String user, final String password) {
+    if (client != null) {
+      client.close();
+      client = null;
+    }
+    final Properties properties = new Properties();
+    properties.setProperty(DrillProperties.USER, user);
+    if (password != null) {
+      properties.setProperty(DrillProperties.PASSWORD, password);
+    }
+    updateClient(properties);
+  }


Let's move these additions to ClientFixture. It will mean that they're no longer drop-in replacements but they'll be more at home there.

Hi James, ClientFixture doesn't have a client variable there, so that we could't close client at first and we need to change updateClient signature from void to return ClientFixture, besides due to cluster variable here is none static, we also need to change updateClient from static method to none static method, and those modifications involve lots of code change. Furthermore, there are still lots of test cases that we could just update client without restart cluster, and for those cases that do need restart cluster, we could use startCluster method to config new cluster and client at same time without using updateClient. So maybe we could still keep those code here?

Ah, yes you're right. These methods still look a bit out of place. Can errorMsgTestHelper move to ClientFixture? And can the new updateClient methods be replaced by usage of cluster.addClientFixture(Properties p) and cluster.client(int number)?

Let me test out a couple of things out locally. If they work I'll send a PR to you, otherwise we'll be ready to merge as is.

Thanks James! I have tried cluster.addClientFixture(), due to this method doesn't copy all the client properties from ClusterFixture, so after create a new ClientFixture, it still has problem to connect with ClusterFixture. And I also tried to modify updateClient method signature to return a ClientFixture, however due to the ClusterFixture and ClientFixture started by startCluster() are hold by two static variables, and updateClient should be a non-static method to refactor into ClientFixture, seems a non-static method return a value to static variable made some errors here, and I am still figuring it out. I think the feasible way should be started ClientFixture and ClusterFixture separately in each test case, so that those two variables are both non-static. The drawback here is the code change should be a little too much, but I can submit a commit to see whether it works

Hi James, I pushed a new commit which refactor updateClient from ClusterTest to ClientFixture.

jnturton · 2023-02-13T10:21:57Z

I think this one is licked! I've requested a review from @cgivre to get it a once over from a pair of eyes belonging to a non-author.

jnturton · 2023-02-13T11:50:32Z

Let me see what's gone awry with the Hive tests...

cgivre · 2023-02-13T12:45:37Z

Let me see what's gone awry with the Hive tests...
@jnturton Did you do a rebase with DRILL-8398 and DRILL-8400?

jnturton · 2023-02-13T13:07:36Z

@jnturton Did you do a rebase with DRILL-8398 and DRILL-8400?

I did think of that and now have, thanks.

jnturton · 2023-02-13T16:08:53Z

The Hive unit tests are quite problematic for me locally due to more problems than you'd expect when I try to do test runs under JDK 8. Anyway, I think that I've fixed the two broken Hive impersonation tests and rebased on master in this new PR to your branch.

kingswanwho · 2023-02-13T16:25:04Z

The Hive unit tests are quite problematic for me locally due to more problems than you'd expect when I try to do test runs under JDK 8. Anyway, I think that I've fixed the two broken Hive impersonation tests and rebased on master in this new PR to your branch.

I have merged and pushed, it should work well this time : -)

cgivre · 2023-02-13T16:54:18Z

@jnturton @kingswanwho I'll review once CI passes later today. Looks good so far!

cgivre · 2023-02-13T19:08:28Z

@kingswanwho Looks like we're still getting some errors on the Hive unit tests. I'm rerunning to see if it was a fluke or not.

cgivre · 2023-02-14T01:35:56Z

@kingswanwho @jnturton It looks like Java 8 is still not liking the Hive unit tests.

kingswanwho · 2023-02-14T01:49:51Z

Hi Charles, Thanks for the remind! Need James' help to investigate.

kingswanwho force-pushed the clean-up-deprecated-code branch from a14dfba to e996f17 Compare March 20, 2022 06:33

luocooong assigned kingswanwho Mar 20, 2022

luocooong added code-cleanup minor-update labels Mar 20, 2022

kingswanwho force-pushed the clean-up-deprecated-code branch 3 times, most recently from 901f77c to 2536a09 Compare March 23, 2022 02:04

paul-rogers reviewed Mar 23, 2022

View reviewed changes

kingswanwho force-pushed the clean-up-deprecated-code branch 5 times, most recently from de3b187 to 20f8848 Compare April 17, 2022 18:47

jnturton requested changes Sep 8, 2022

View reviewed changes

kingswanwho force-pushed the clean-up-deprecated-code branch from 222488f to b757afc Compare September 12, 2022 13:37

jnturton marked this pull request as draft September 13, 2022 15:36

kingswanwho force-pushed the clean-up-deprecated-code branch from b757afc to f9e4988 Compare September 14, 2022 08:16

kingswanwho force-pushed the clean-up-deprecated-code branch 2 times, most recently from b888503 to dac8dff Compare December 28, 2022 10:27

kingswanwho force-pushed the clean-up-deprecated-code branch 5 times, most recently from 056486a to 3ae2951 Compare January 17, 2023 03:32

kingswanwho force-pushed the clean-up-deprecated-code branch 2 times, most recently from 9b9e422 to 993af0e Compare January 31, 2023 06:52

kingswanwho requested a review from jnturton February 1, 2023 09:04

jnturton requested changes Feb 1, 2023

View reviewed changes

kingswanwho force-pushed the clean-up-deprecated-code branch 3 times, most recently from 8d3b329 to 5b5eaae Compare February 7, 2023 02:34

kingswanwho requested a review from jnturton February 7, 2023 04:26

cgivre removed the minor-update label Feb 10, 2023

kingswanwho force-pushed the clean-up-deprecated-code branch 5 times, most recently from 7aad70c to 829381f Compare February 13, 2023 10:15

jnturton approved these changes Feb 13, 2023

View reviewed changes

jnturton requested a review from cgivre February 13, 2023 10:19

kingswanwho force-pushed the clean-up-deprecated-code branch 2 times, most recently from e66fd2d to e1722ab Compare February 13, 2023 16:21

kingswanwho force-pushed the clean-up-deprecated-code branch from e1722ab to b3bc923 Compare February 13, 2023 16:35

DRILL-8117: Clean up deprecated Apache code in Drill

a0855aa

kingswanwho force-pushed the clean-up-deprecated-code branch from b3bc923 to a0855aa Compare February 14, 2023 10:05

kingswanwho closed this Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRILL-8117: Upgrade unit tests to the cluster fixture framework #2499

DRILL-8117: Upgrade unit tests to the cluster fixture framework #2499

kingswanwho commented Mar 20, 2022 •

edited

paul-rogers Mar 23, 2022

kingswanwho Mar 23, 2022

paul-rogers Mar 24, 2022

kingswanwho Mar 28, 2022 •

edited

kingswanwho Apr 17, 2022

cgivre commented Jul 8, 2022

jnturton commented Jul 11, 2022

cgivre commented Jul 19, 2022

jnturton commented Sep 8, 2022

jnturton Sep 8, 2022

kingswanwho Sep 12, 2022 •

edited

jnturton Sep 12, 2022

kingswanwho Sep 13, 2022

jnturton Sep 13, 2022

jnturton Feb 1, 2023

kingswanwho Feb 2, 2023 •

edited

jnturton Feb 2, 2023

jnturton Feb 3, 2023

kingswanwho Feb 5, 2023 •

edited

kingswanwho Feb 6, 2023 •

edited

jnturton commented Feb 13, 2023

jnturton commented Feb 13, 2023

cgivre commented Feb 13, 2023

jnturton commented Feb 13, 2023

jnturton commented Feb 13, 2023

kingswanwho commented Feb 13, 2023

cgivre commented Feb 13, 2023

cgivre commented Feb 13, 2023

cgivre commented Feb 14, 2023

kingswanwho commented Feb 14, 2023 •

edited

DRILL-8117: Upgrade unit tests to the cluster fixture framework #2499

DRILL-8117: Upgrade unit tests to the cluster fixture framework #2499

Conversation

kingswanwho commented Mar 20, 2022 • edited

DRILL-8117: DRILL-8117: Upgrade unit tests to the cluster fixture framework

Description

Documentation

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingswanwho Mar 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgivre commented Jul 8, 2022

jnturton commented Jul 11, 2022

cgivre commented Jul 19, 2022

jnturton commented Sep 8, 2022

Choose a reason for hiding this comment

kingswanwho Sep 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingswanwho Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingswanwho Feb 5, 2023 • edited

Choose a reason for hiding this comment

kingswanwho Feb 6, 2023 • edited

Choose a reason for hiding this comment

jnturton commented Feb 13, 2023

jnturton commented Feb 13, 2023

cgivre commented Feb 13, 2023

jnturton commented Feb 13, 2023

jnturton commented Feb 13, 2023

kingswanwho commented Feb 13, 2023

cgivre commented Feb 13, 2023

cgivre commented Feb 13, 2023

cgivre commented Feb 14, 2023

kingswanwho commented Feb 14, 2023 • edited

kingswanwho commented Mar 20, 2022 •

edited

kingswanwho Mar 28, 2022 •

edited

kingswanwho Sep 12, 2022 •

edited

kingswanwho Feb 2, 2023 •

edited

kingswanwho Feb 5, 2023 •

edited

kingswanwho Feb 6, 2023 •

edited

kingswanwho commented Feb 14, 2023 •

edited