You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/06/29 09:50:18 UTC
[GitHub] [druid] rohangarg opened a new pull request, #12716: Fix skipTests build flag
rohangarg opened a new pull request, #12716:
URL: https://github.com/apache/druid/pull/12716
Fixes the `-DskipTests` flag to skip UTs and ITs when given via command line
For console, added the change since I think the tests (test-console goal) are launched by the frontend plugin which doesn't understand `skipUTs` flag.
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #12716:
URL: https://github.com/apache/druid/pull/12716#issuecomment-1170255932
Thanks for catching the console issue!
This one is tricky! The `skipTests` flag is used by Surefire. It can be set via the command line, via a Maven property, or as a plugin property. If you say `-DskipTests`, then you override anything set in the `pom.xml` files.
We're trying to move to revised ITs, based on Failsafe, which shares code with Surefire. One of the things it shares is the `skipTests` property. So, if we set `skipTests`, we disable both the ITs and UTs.
A bit of StackOverflow searching revealed a workaround: define two separate properties: `skipUTs` for unit tests (Surefire) and `skipITs` for integration tests (Failsafe). Then, in the `pom.xml`:
```xml
<artifactId>maven-surefire-plugin</artifactId>
<skipTests>${skipUTs}</skipTests>
```
Now, you can still force it with `-DskipTests`, which overrides the above.
With the revised ITs, you will want the fine tuning. The PR that this PR fixes tried multiple combinations. The present code was the one which _seemed_ to work. This PR points out that the console was broken.
This fix adds `skipTests` back. By so doing, we complicate the process: it will disable both test systems, just so we can disable the console tests. What we really want is to decide if those tests are considered UTs or ITs. If UTs, then the web console `pom.xml` file change is correct.
If we make that change, then we don't need the other changes. Setting `skipTests` in the `pom.xml` doesn't do anything: we already control Surefilre as shown above. From the command line, `-DskipTests` should override the `pom.xml`, since options set on the command line override anything set in the `pom.xml` file.
At the time of this writing, the build hasn't completed. When I mucked with this, I ran into several issues. First, one cannot reference an undefined property. So, just doing the following:
```xml
<skipUTs>${skipTests}</skipUTs>
<skipITs>${skipTests}</skipITs>
```
Will fail the build if `skipTests` is unset.
Also, there is a timing issue. Properties are set at the start of the build. Profiles come later. So, a profile cannot set a property which is referenced in the properties section (IIRC.). This, proposed change will probably not work: first we set the properties based on `skipTests=false`, then we later change `skipTests=true`, but Maven isn't smart enough to recompute `skipITs`, etc. I think I recall something about early- vs. late-binding properties. So, I'll caveat the above comment by saying I encountered the issue, went the route in the current code, and didn't dig into the binding reference.
Bottom line:
* The web console fix is good. Thanks for catching that.
* The `pom.xml` changes shouldn't be needed.
* Setting `-DskipTest` on the command line should work. If it doesn't, I'm missing something (which is likely, Maven is hard to understand.)
* Setting `-P skip-tests` on the command line is what Travis does and seems to be the "official" way to skip tests.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12716:
URL: https://github.com/apache/druid/pull/12716#issuecomment-1170183592
Wow, I didn't misclick. Github changed the default action to "close with comment" instead of "comment" 🙂
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
clintropolis merged PR #12716:
URL: https://github.com/apache/druid/pull/12716
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12716:
URL: https://github.com/apache/druid/pull/12716#issuecomment-1170183901
Can't stop hitting command-enter, I guess.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12716:
URL: https://github.com/apache/druid/pull/12716#issuecomment-1170182908
Oops, sorry, I misclicked "close".
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12716:
URL: https://github.com/apache/druid/pull/12716#issuecomment-1170182500
With this change do `skipUTs` and `skipITs` still work independently?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm closed pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
gianm closed pull request #12716: Fix skipTests build flag
URL: https://github.com/apache/druid/pull/12716
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on pull request #12716: Fix skipTests build flag
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #12716:
URL: https://github.com/apache/druid/pull/12716#issuecomment-1170550543
Thanks for the offline discussion. Sounds like this change works for the use cases in question. Let's go ahead and commit it (non-binding), the I can work out any issues in the "new IT" branch separately. Thanks for finding the issue and making the fix!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org