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 2021/05/21 08:32:16 UTC
[GitHub] [druid] clintropolis opened a new pull request #11283: Chill travis
clintropolis opened a new pull request #11283:
URL: https://github.com/apache/druid/pull/11283
Migrating #11238 to a branch on the apache repo so I can run more tests before we merge.
>Initial attempt at making travis be a bit more selective about what it runs, since every change takes like.. over a day of compute time.
>This PR adds a new script, `check-test-suite.py`, which is used in `.travis.yml` jobs to check whether or not a given job should run. It does this based on an admittedly primitive git diff file path prefix matching against stuff hard coded into the script, initially dividing into 3 categories: 'docs', 'web-console' and like, everything else. We do some similar stuff for code coverage diff checking enforcement for java projects, so a follow-up improvement could provide finer granularity for the 'everything else' bucket.
>I'm sure there is a better way to configure this than hard coding stuff into the script as well, but, its a start and should let us at least cut down a tiny amount of the global warming we are surely causing with all of this CI.
>I think the only thing that is questionable is considering changes to `.travis.yml` skippable, which will result in newly added configurations of existing tests not getting triggered I think, but maybe that isn't a common situation? Also, I have the license checks and packaging checks running always, because they seem like useful sanity checks to make sure all license stuff is cool and that a distribution can be built from source.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11283: chill, travis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11283:
URL: https://github.com/apache/druid/pull/11283#discussion_r640487822
##########
File path: check_test_suite.py
##########
@@ -0,0 +1,135 @@
+#!/usr/bin/env python3
+
+# 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.
+
+import os
+import subprocess
+import sys
+
+# do some primitive examination of git diff to determine if a test suite needs to be run or not
+
+always_run_jobs = ['license checks', '(openjdk8) packaging check', '(openjdk11) packaging check']
Review comment:
oops fixed, and added a bunch of other comments too
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #11283: chill, travis
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #11283:
URL: https://github.com/apache/druid/pull/11283#discussion_r638419944
##########
File path: check_test_suite.py
##########
@@ -0,0 +1,135 @@
+#!/usr/bin/env python3
+
+# 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.
+
+import os
+import subprocess
+import sys
+
+# do some primitive examination of git diff to determine if a test suite needs to be run or not
+
+always_run_jobs = ['license checks', '(openjdk8) packaging check', '(openjdk11) packaging check']
Review comment:
The comment above doesn't seem to match what this variable is used for. I think the comment above is for the whole file, and this variable has no comment - is this assumption correct? Could we make it clearer in here if that's the case?
I think something as simple as re-wording the previous comment to `# This script is meant to do some primitive...`
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #11283: Chill travis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11283:
URL: https://github.com/apache/druid/pull/11283#issuecomment-846329402
This script appears to be working correctly so far on the 3 example PRs that are linked to this one.
The java only change ran most of the test suites, but skipped the web-console, web-console e2e, and docs tests, https://travis-ci.com/github/apache/druid/builds/226501655
https://travis-ci.com/github/apache/druid/jobs/506826045#L667
The web-console correctly test skipped all jobs except web-console and web-console e2e tests, https://travis-ci.com/github/apache/druid/builds/226501846 cutting total CI compute time down from over 24 hours to under 2.5 hrs (~30 minutes of wall time)
The 3rd PR contains a first commit that is only a doc change, where all tests were skipped except for the docs test https://travis-ci.com/github/apache/druid/builds/226501778, using ~2 hr of compute time, but then a second commit was added to a java file which has triggered all of the java tests to run in addition to the docs tests (to ensure that the diff from all of the PR commits is what we see due to how travis runs PR builds, instead of just the last commit as the git command our script is using might suggest, see https://travis-ci.com/github/apache/druid/jobs/507046243#L662 which has the files from both commits).
The branch build ran the entire test suite and skipped nothing, https://travis-ci.com/github/apache/druid/builds/226501106, though looking at the log I guess I could stand to change the script a bit to print that the reason for not skipping the tests is because it is not a PR build to make clearer the reason the tests are being run.
I also added a unit test for this test skipping script, "script checks", https://travis-ci.com/github/apache/druid/jobs/506826488, since it was already getting a bit complicated and will only continue to do so as we add more functionality to this script to more selectively run the java 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.
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] suneet-s commented on pull request #11283: chill, travis
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11283:
URL: https://github.com/apache/druid/pull/11283#issuecomment-849599934
> @suneet-s did you catch this comment in the previous PR [#11238 (comment)](https://github.com/apache/druid/pull/11238#discussion_r636631764)? tl;dr It mentions that all the commits in a travis PR run are squashed on top of the target branch. You can see this happening in the 2nd run of the 3rd PR, https://travis-ci.com/github/apache/druid/jobs/507046243#L662,
Thanks for pointing this out... I had missed that comment. The list of files that have changed in the logs are very convincing. No need for extra 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.
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 commented on pull request #11283: chill, travis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11283:
URL: https://github.com/apache/druid/pull/11283#issuecomment-849519311
>@clintropolis can you push a few more commits to your test PRs to show what happens when there are multiple commits in a PR. Once these tests do what we expect, I'm LGTM.
@suneet-s did you catch this comment in the previous PR https://github.com/apache/druid/pull/11238#discussion_r636631764? tl;dr It mentions that all the commits in a travis PR run are squashed on top of the target branch. You can see this happening in the 2nd run of the 3rd PR, https://travis-ci.com/github/apache/druid/jobs/507046243#L662,
```
$ ./check_test_suite.py && travis_terminate 0 || echo 'Running maven install...' && MAVEN_OPTS='-Xmx3000m' travis_wait 15 ${MVN} clean install -q -ff -pl '!distribution' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS} -T1C && ${MVN} install -q -ff -pl 'distribution' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS}
Checking if suite 'checkstyle' needs to run test on diff:
core/src/main/java/org/apache/druid/math/expr/InputBindings.java
docs/development/experimental.md
```
where the files printed when running the script are from both of the commits (each commit had 1 file change), so it doesn't really seem necessary to me to run the additional tests you've suggested, but can do it if you're not convinced 😅
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #11283: chill, travis
Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #11283:
URL: https://github.com/apache/druid/pull/11283
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis edited a comment on pull request #11283: Chill travis
Posted by GitBox <gi...@apache.org>.
clintropolis edited a comment on pull request #11283:
URL: https://github.com/apache/druid/pull/11283#issuecomment-846329402
This script appears to be working correctly so far on the 3 example PRs that are linked to this one.
The java only change ran most of the test suites, but skipped the web-console, web-console e2e, and docs tests, https://travis-ci.com/github/apache/druid/builds/226501655
https://travis-ci.com/github/apache/druid/jobs/506826045#L667
The web-console correctly test skipped all jobs except web-console and web-console e2e tests, https://travis-ci.com/github/apache/druid/builds/226501846 cutting total CI compute time down from over 24 hours to under 2.5 hrs.
The 3rd PR contains a first commit that is only a doc change, where all tests were skipped except for the docs test https://travis-ci.com/github/apache/druid/builds/226501778, using ~2 hr of compute time, but then a second commit was added to a java file which has triggered all of the java tests to run in addition to the docs tests (to ensure that the diff from all of the PR commits is what we see due to how travis runs PR builds, instead of just the last commit as the git command our script is using might suggest, see https://travis-ci.com/github/apache/druid/jobs/507046243#L662 which has the files from both commits).
The branch build ran the entire test suite and skipped nothing, https://travis-ci.com/github/apache/druid/builds/226501106, though looking at the log I guess I could stand to change the script a bit to print that the reason for not skipping the tests is because it is not a PR build to make clearer the reason the tests are being run.
I also added a unit test for this test skipping script, "script checks", https://travis-ci.com/github/apache/druid/jobs/506826488, since it was already getting a bit complicated and will only continue to do so as we add more functionality to this script to more selectively run the java 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.
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] suneet-s commented on pull request #11283: chill, travis
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #11283:
URL: https://github.com/apache/druid/pull/11283#issuecomment-847496965
@clintropolis can you push a few more commits to your test PRs to show what happens when there are multiple commits in a PR.
A couple of scenarios I'd like to better understand what tests will run -
* #11284 - just java changes in one commit + a docs change in the latest commit (the reverse of #11285 to make sure that it looks at all the changes in the PR and not just the last one)
* #11285 revert the java changes and push another patch to the PR - should just the doc tests run?
* docs and web console changes only either in 1 commit or consecutive commits
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s edited a comment on pull request #11283: chill, travis
Posted by GitBox <gi...@apache.org>.
suneet-s edited a comment on pull request #11283:
URL: https://github.com/apache/druid/pull/11283#issuecomment-847496965
@clintropolis can you push a few more commits to your test PRs to show what happens when there are multiple commits in a PR. Once these tests do what we expect, I'm LGTM.
A doc update somewhere to explain which tests are expected to run in what scenarios would be a nice to have, but for now, since only devs are looking at this, I think this PR is pretty self documenting.
A couple of scenarios I'd like to better understand what tests will run -
* #11284 - just java changes in one commit + a docs change in the latest commit (the reverse of #11285 to make sure that it looks at all the changes in the PR and not just the last one)
* #11285 revert the java changes and push another patch to the PR - should just the doc tests run?
* docs and web console changes only either in 1 commit or consecutive commits
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org