Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KT-64377 use Java Test Suites for Gradle integration tests #3427
KT-64377 use Java Test Suites for Gradle integration tests #3427
Changes from 31 commits
c31dd2361889af74a1b2020e098466287db28f4d7a7af039410fe4b55f35a730babb2c9c6c80eb1c95de28a1c748a24b4d57b93cfa974d9b852fc7bba0c4846d5ab6314364ea84255e237581f2f6d1cca9fe882b59594ddd249e64ed0365f50a6ef499fc75347b3dc8cd554b0668c661939d57742cd06cdf52ca16eff18d9ec851437a38fbb1c9a41f09111eFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK we already have this dependency below:
addDependencyOnSameTasksOfIncludedBuildsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in this root build script we have
integrationTestaggregate task, which, as far as I understand, will now run only maven and cli tests and not Gradle. Are we planning to migratemaven/clitests to test suites soon? If no, I think it will be better to somehow align this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: before, when running
check(gradle check) integration tests were not executed, but now they are. It's not convenient as integration tests are long :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately
addDependencyOnSameTasksOfIncludedBuildsadds a dependency on:dokka-integration-tests:check, not:dokka-integration-tests:gradle:check.TBH I found the logic about setting up task dependencies a bit confusing in this file, so there's probably a better way of using the functions, but I found being explicit easier to understand :).
I get that it's a workaround for a Gradle flaw gradle/gradle#22335 - I think using a task rule might be help https://stackoverflow.com/a/69179714/4161471
Yes, for sure. I wanted to do it one subproject at a time, to minimize PR size, and then it will be easier to determine what functionality can be extracted into conventions & build-logic utils.
Yes, that's intentional :) 'check' is the defacto standard lifecycle task for all verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start from the end:)
test+apiCheck(so oldcheck) to test if my changes didn't broke anything. Sometimes it's fine to run only one module/test, but not all the time. At my side runningchecknow will take 1-2 minute I think. If we will put running integration tests intocheck- it will become much more, so I will start to usetestandapiChecktasks separately. Or I will just runcheckfor some subproject(s). External contributors also could just runcheck(as it'sdefacto standard lifecycle task for all verification.) and Im not sure, that all of them will want to wait for all tests or some of the tests could even fail, f.e. android, if there is no setup, and they will spend more time to fix the issue or will just ignore running test, or even finishing the feature. This is a little inconvenient.check- and I think for a reason.checkis day-to-day task for fast versification - integration tests are not fast.So I think, that running
checkshould not run integration tests - they should run by default on CI (in same/separate workflow), it should be enough for most cases. If needed, they can be easily run locally, all of them (rarely) or specific one (more frequently).Agree, though, I've just mentioned inconsistency in behaviour of different integration tests after merging this PR - not a blocker, but just something to be aware of.
yeah, still, it's very convenient to have such
aggregatedtasks in root project. If you have ideas on how to improve this, I will be glad to review separate PR :)I've never used task rules before, so I don't know will it be possible/simpler/complex, so up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining, it definitely makes sense to make the UX of the Gradle tasks more comprehensible. Anyone should be able to run suitable tests with a single Gradle task.
I see there are some good options for this, proposed at the bottom, but first I want to clarify about the
checktask.Check task
Personally, I find the current behaviour (
checkdoesn't run all tests) confusing because in all other projects it's a lifecycle task that runs all verifications.That's because the
checktask is provided by the Gradle Base Plugin, which all other plugins use, and it's supposed to run all verifications.Of course, Gradle is very flexible so Dokka doesn't have to follow this convention, but it would be unusual. It's important to align the conventions within Kotlin projects - so while "check runs all tests" might not meet your understanding at the moment, I hope it will make more sense in the future.
Proposal
I see two options
Update the
testtask so that it will also run theapiChecktask.Update the README instructions:
./gradlew check- run all verifications (which might be slow):./gradlew test- run unit tests (and alsoapiCheck)./gradlew apiCheck- BCV check./gradlew integrationTest- run all integration tests (Slow! Requires Prep!)Add a custom
quickCheck(orcheckFast- whatever name you want) lifecycle task that will run unit tests + apiCheck (and any other future 'fast' checks).Update the README instructions:
./gradlew check- run all verification (which might be slow):./gradlew quickCheck- runs quicker checks (testandapiCheck)./gradlew test- run unit tests./gradlew apiCheck- BCV check./gradlew integrationTest- run all integration tests (Slow! Requires Prep!)I have a slight preference towards 1, because currently there's not many verification tasks apart from
testandapiCheckthat would justify a new task.Which do you prefer?
I can also split out these changes to the test-task dependencies into a separate PR to keep this PR focused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to define "all" here? :)
I think it's very important to draw the line somewhere, otherwise it's easy to fall into unproductive malicious compliance and make the
checktask perform literally all verifications:kotlin-compiler/ KGP / etc (also planned)I would draw the line somewhere close to "Runs all embedded verifications that don't require special knowledge or setup, and that are reasonably expected to be run locally by developers". By that definition, Dokka's "integration tests" (which are really completely separate e2e tests) are not it for several reasons
About the proposals - I would first try to understand why we need the
checktask to run Dokka's "integration tests" specifically. It's taken for granted, but I don't think it should be :)Who or what is going to be using it if
checkalso runs Dokka's "integration tests"? How is it going to be useful? (other than abiding by Gradle's "all checks", which can be taken to extremes)Certainly wouldn't hurt, if you have the time 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Im totally agree with Ignat.
Still I will add some additional points:
checkdependsOn instrumented tests (which are executed on emulator/real device) - they are slow and environment dependentchecktestdependOnapiCheckor any other check task (f.e. if we will adddetekt) - if we will run tests from IDEA via gutter (just one test or test class), we will executeapiCheck- not sure it's expected. May be it's possible to workaround it, but not sure that by doing this we will simplify both build setup and user expectations.So I believe
integrationTestsfor Dokka should be executed in separate task. Additionally, we could created lifecycle taskcheckAll/checkLongif we really do want to execute it.Another possibility: make global
checkrun all tests except integration tests. makedokka-integration-testscheckrun all tests there, even integration.It can cause confusion, in this case we will have some split between
checktasks. But IMO, it will be still better then running allintegration testsby calling globalcheckThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it would be more conventional to have the
checktask that tests as much as possible within the current environment and thus you just need to invoke a single task and wait; also I agree with Adam that something likequickCheckorsmokeCheckcould be used here for smoke testing. It's about making the build conventional and thus easing the build discoverability. But, also I agree that we shouldn't always blindly follow the conventions, especially if it harms locally established use cases. Anyway, the final word is on your side 🙂After a round of thinking, I don't see anything criminal in breaking that convention a bit. We are not the first, we are not the last here. I would just suggest to reflect this in the project docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the check task dependency.
See https://docs.gradle.org/current/userguide/base_plugin.html
There are no Gradle tasks for this, so
checkcan't depend on them.checkcan't depend on tasks in other repos.This is a good idea. It would help prevent the problem we saw recently where the K2 tests weren't running on TeamCity.
If this is achieved with a Gradle task then yes,
checkshould depend on that task.This depends:
If the task has checks, then yes,
checkshould depend on itFor example, the existing
SequentialTasksExecutionStressTestIf the tasks are measuring performance and reporting on it, but not verifying it, then that's not a verification task so
checkshouldn't depend on it.For example: if JMH tests were added
I agree, there's a good need for such lifecycle task, and the meaning you explain is clear.
It's confusing to overload an existing task with a different meaning. It would make more sense to create a new lifecycle task that meets this definition.
It would also help prevent issues on CI where verifications have been skipped. E.g. K2 tests not being run, and up until recently apiCheck wasn't run on TeamCity.
Using
checkto run all test tasks aligns howcheckworks both within JetBrains, but also within the Gradle ecosystem. Many other plugins usecheckto run all verifications.