Skip to content

chill, travis#11283

Merged
suneet-s merged 10 commits intomasterfrom
chill-travis
May 27, 2021
Merged

chill, travis#11283
suneet-s merged 10 commits intomasterfrom
chill-travis

Conversation

@clintropolis
Copy link
Member

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.

@clintropolis
Copy link
Member Author

clintropolis commented May 22, 2021

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.

@clintropolis clintropolis changed the title Chill travis chill, travis May 22, 2021
@suneet-s
Copy link
Contributor

suneet-s commented May 25, 2021

@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 -


# 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']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops fixed, and added a bunch of other comments too

@clintropolis
Copy link
Member Author

@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 #11238 (comment)? 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 😅

@suneet-s
Copy link
Contributor

@suneet-s did you catch this comment in the previous PR #11238 (comment)? 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

Copy link
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@suneet-s suneet-s merged commit 7a2bcb3 into master May 27, 2021
@suneet-s suneet-s deleted the chill-travis branch May 27, 2021 12:40
zachjsh pushed a commit to zachjsh/druid that referenced this pull request Jun 16, 2021
* Add test for join on __time column (apache#11289)

* chill, travis (apache#11283)

* maybe make ci more selective in what it do

* maybe this

* skip self and travis config

* can it be in before_script? lets find out

* nope

* fixes

* oops

* only skip stuff for PR builds, add some tests

* revert unintended change

* more comments, more tests, more better

* Add configuration suggestion to `druid.indexer.storage.type` (apache#11304)

Co-authored-by: Rohan Garg <7731512+rohangarg@users.noreply.github.com>
Co-authored-by: Clint Wylie <cwylie@apache.org>
Co-authored-by: frank chen <frank.chen021@outlook.com>
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Dev For items related to the project itself, like dev docs and checklists, but not CI Area - Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants