[#2279] improvement(test): Isolate catalog class path in IT

[yuqi1129]

What changes were proposed in this pull request?

Move the test cases that depend on the Docker environment to their corresponding catalog modules.

Why are the changes necessary?

For better management.

Fix: #2279

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

Existing test can cover the changes.

[jerryshao]

@yuqi1129 I would suggest we have a thorough solution before doing this change.

[yuqi1129]

@yuqi1129 I would suggest we have a thorough solution before doing this change.

Let me thoroughly review the hive-related tests. It appears that the hive-related tests have more complex dependencies:

• HDFS. This is to check the file's existence and read/write status. For example, drop an external table and verify if the path of the table is clear.
• Hive Metastore was used to check the status of the Hive Docker instance, retrieve Hive table objects through Thrift, and then compare them with that loaded by Gravitino.
• Thrift: Handle some exceptions.

Iceberg and JDBC-related tests are clear and under control.

[jerryshao]

Issue has nothing related to your PR, please fix the issue. Also adding detailed descriptions to issue.

[jerryshao]

The code is far from ready, please spend time on polishing it, and think about the overall code organizations.

[jerryshao]

Have you verified locally?

[yuqi1129]

Yes, I have verified the changes locally.

[jerryshao]

You should also check why tasks.configureEach<Test> in root gradle is not worked. Also, if the tasks.configureEach<Test> is not worked, it means other codes here is also not worked, like setting testNG and others, you should confirm all.

[jerryshao]

Also, I think you need to keep this behavior after the change

  tasks.configureEach<Test> {
    testLogging {
      exceptionFormat = TestExceptionFormat.FULL
      showExceptions = true
      showCauses = true
      showStackTraces = true
    }
    reports.html.outputLocation.set(file("${rootProject.projectDir}/build/reports/"))
    val skipTests = project.hasProperty("skipTests")
    if (!skipTests) {
      if (project.name == "trino-connector") {
        useTestNG()
        maxHeapSize = "2G"
      } else {
        useJUnitPlatform()
      }

      jvmArgs(project.property("extraJvmArgs") as List<*>)
      finalizedBy(tasks.getByName("jacocoTestReport"))
    }
  }

[yuqi1129]

You should also check why tasks.configureEach<Test> in root gradle is not worked. Also, if the tasks.configureEach<Test> is not worked, it means other codes here is also not worked, like setting testNG and others, you should confirm all.

Maybe we need to clarify what the skipTests means. In the main branch, skipTests will skip all tests in the module, it's equivalent to skipping all unit tests, however, when we move ITs to the module, I want to confirm what the behavior skipTests we need to abide by.

The latest changes mean we will skip all unit tests if users set property skipTests.

Maybe I need to revert the latest changes here as the syntax skipTests in module integration-test and catalogs-related test is not the same? @jerryshao

You should also check why tasks.configureEach<Test> in root gradle is not worked

It works actually if skipTests means skipping all tests including ITs.

[yuqi1129]

Also, I think you need to keep this behavior after the change

  tasks.configureEach<Test> {
    testLogging {
      exceptionFormat = TestExceptionFormat.FULL
      showExceptions = true
      showCauses = true
      showStackTraces = true
    }
    reports.html.outputLocation.set(file("${rootProject.projectDir}/build/reports/"))
    val skipTests = project.hasProperty("skipTests")
    if (!skipTests) {
      if (project.name == "trino-connector") {
        useTestNG()
        maxHeapSize = "2G"
      } else {
        useJUnitPlatform()
      }

      jvmArgs(project.property("extraJvmArgs") as List<*>)
      finalizedBy(tasks.getByName("jacocoTestReport"))
    }
  }

This behavior has not changed.

[jerryshao]

You should also check why tasks.configureEach<Test> in root gradle is not worked. Also, if the tasks.configureEach<Test> is not worked, it means other codes here is also not worked, like setting testNG and others, you should confirm all.

Maybe we need to clarify what the skipTests means. In the main branch, skipTests will skip all tests in the module, it's equivalent to skipping all unit tests, however, when we move ITs to the module, I want to confirm what the behavior skipTests we need to abide by.

The latest changes means we will skip all unit tests if users set property skipTests.

You should also check why tasks.configureEach<Test> in root gradle is not worked

It works actually if skipTests means skipping all tests including ITs.

Let's keep this behavior.

[yuqi1129]

Let's keep this behavior.

Okay, I will revert the latest changes.

[jerryshao]

I mean keep what current main branch's definition: skipTests => skip UT, skipITs => skip IT, not reverting it. You can check the ci script, currently there's no chance to run ITs in each catalog.

You should think about which way is the right way, not just changing it and then reverting it abruptly.

[yuqi1129]

I mean keep what current main branch's definition: skipTests => skip UT, skipITs => skip IT, not reverting it. You can check the ci script, currently there's no chance to run ITs in each catalog.

You should think about which way is the right way, not just changing it and then reverting it abruptly.

I see. I will use another PR to change the name skipTests to skipUTs, the current name is quite misleading.

[yuqi1129]

Each sub-module needs to configure useJUnitPlatform() to make the UTs work well, that is how skipTests is working.

[jerryshao]

Thanks @yuqi1129 for your work, please keep improving if there's something missing or need to refactor further.

[jerryshao]

I mean keep what current main branch's definition: skipTests => skip UT, skipITs => skip IT, not reverting it. You can check the ci script, currently there's no chance to run ITs in each catalog.
You should think about which way is the right way, not just changing it and then reverting it abruptly.

I see. I will use another PR to change the name skipTests to skipUTs, the current name is quite misleading.

It's actually the way maven uses, maven uses skipTests to skip all the UTs.

[yuqi1129]

I mean keep what current main branch's definition: skipTests => skip UT, skipITs => skip IT, not reverting it. You can check the ci script, currently there's no chance to run ITs in each catalog.
You should think about which way is the right way, not just changing it and then reverting it abruptly.

I see. I will use another PR to change the name skipTests to skipUTs, the current name is quite misleading.

It's actually the way maven uses, maven uses skipTests to skip all the UTs.

To the best of my ability, SkipTests will skip all tests other than UTs.