IT-related changes pulled out of PR #12368 #12673
Conversation
There was a problem hiding this comment.
LGTM!! Thanks, @paul-rogers for taking the lead and fixing up the IT's.
integration-tests/src/main/java/org/apache/druid/cli/CustomNodeRoleCommandCreator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the detailed description, @paul-rogers !
I have added some minor comments.
.travis.yml
Outdated
| @@ -34,8 +34,8 @@ env: | |||
| - DOCKER_IP=127.0.0.1 # for integration tests | |||
| - MVN="mvn -B" | |||
| - > # Various options to make execution of maven goals faster (e.g., mvn install) | |||
| MAVEN_SKIP="-Pskip-static-checks -Ddruid.console.skip=true -Dmaven.javadoc.skip=true" | |||
| - MAVEN_SKIP_TESTS="-Pskip-tests" | |||
| MAVEN_SKIP="-P skip-static-checks -Ddruid.console.skip=true -Dmaven.javadoc.skip=true" | |||
integration-tests/README.md
Outdated
| Add `-rf :druid-integration-tests` when running integration tests for the second time or later without changing | ||
| Parameters: | ||
|
|
||
| * Test Group: Required, as certain test setup and cleanup tasks are based on the test group. You can find |
There was a problem hiding this comment.
Nit:
| * Test Group: Required, as certain test setup and cleanup tasks are based on the test group. You can find | |
| * Test Group: Required, as certain test setups and cleanup tasks are based on the test group. You can find |
There was a problem hiding this comment.
Perhaps this is ambiguous. I read this as "setup ... tasks". You maybe saw it as "setups AND (cleanup tasks)". Reworded to avoid the ambiguity.
integration-tests/README.md
Outdated
| Note that when bringing up Docker containers through Maven and `-Doverride.config.path` is provided, additional | ||
| Druid routers for security group integration test (permissive tls, no client auth tls, custom check tls) will not be started. | ||
|
|
||
| ### Debugging Test Runs |
There was a problem hiding this comment.
Side note: All of these seem to be very helpful bits of information. I wonder if we should have a "Developer" section in the Druid docs and capture all such information there too. I sometimes find it easier to read docs than have to open the README on GitHub or on an IDE.
There was a problem hiding this comment.
Great suggestion. To avoid redundant work, let's do that when we merge the "new" IT framework, since the steps will differ.
|
Build is basically clean except for an unrelated IT failure. @kfaraz please either restart that test, or use your judgment that the failure is not relevant to the changes here. |
|
Thanks for the changes, @paul-rogers . I have merged the PR. |
The new IT PR has struggled to get a clean build due to the complexity and flakiness of the Druid build system. It is ironic that the attempt to fix the ITs has been stymied by the fragility of the existing ITs. Out of desperation, the big PR is being broken up into smaller chunks. This one provides changes made to the existing ITs to support the new ITs.
The first set of changes make the "custom node role" code usable by the new ITs. No functional change, just some clean-up. There are also some documentation updates.
The second change is more substantial. The new ITs use the "failsafe" Maven plugin, which is the sister plugin to the "failsafe" plugin Druid uses for unit tests (UTs). The venerable
-DskipTestsflag will thus skip both ITs and UTs. But, when running the new ITs, we want to skip the UTs, but run the ITs. A common solution is to use separate flags for ITs and UTs.-DskipITsskips the integration tests but runs unit tests.-DskipUTsskips unit tests but runs the "new" integration tests.The existing Druid profile,
-P skip-testsis expanded to skip both ITs and UTs.In this PR,
-DskipTestsand-DskipUTshave the same effect. However, once the new ITs are in place, the meanings will differ, as explained above.The key purpose of this PR is to test the new setup in Travis separately from the other changes in PR #12368.
This PR has: