search

Kotlin / dokka

API documentation engine for Kotlin
https://kotl.in/dokka
Apache License 2.0
3.45k stars 410 forks source link

Run `check` task on CI #3808

Closed whyoleg closed 1 month ago

whyoleg commented 2 months ago

fixes https://github.com/Kotlin/dokka/issues/3805

Introduce a Gradle property to skip integration tests, which could be set on CI/locally to be able to run check task without running all integration tests.

If we will enable running integration tests unconditionally we will increase GA tests time from 10-15 minutes to more than an hour (example). Note: all tests are red in the above example because most of them failed with OOM or "not enough space" when downloading K/N toolchain for different version of Kotlin. So enabling integration tests unconditionally on GA is not an option also for reasons other than "slow tests".

Note: running check on CI exposed issue with Windows and DGPv2 function tests. This should be fixed before the merging of this PR. Additionally after the PR is merged we could change TC setup for Dokka Tests to also use check with integration_test.skip=true flag. Or even drop it, as it doesn't cover other OS like we have on GA.

whyoleg commented 2 months ago

Hm, running check causes tests with K1 runs twice, because we have both test and testDescriptors do run tests with K1 :(

And looks like there is no good way to tell both Gradle and IDEA that we want to have test to just run both testDescriptors and testSymbols but not run anything by itself...

Tapchicoma commented 2 months ago

@whyoleg maybe you should structure your tests using Gradle test-suite plugin and then via Gradle properties just disable some suites

whyoleg commented 2 months ago

This would help with test reporting, as the tests would be reported as 'skipped' in places like Build Scan and TeamCity.

just curious: this is needed to understand if the test is skipped or just not run? I can add this tag, but adding it to every test in integration tests feels not right :)

Instead of another custom workaround, maybe just remove testDescriptors?

I've decided to just skip test task running and make it aggregate. In the end, looks like even now running test from IDEA works bad: as no filters are applied to testSymbols and so it runs all tests even if only one test selected.

Rather than dynamically enabling/disabling tasks, I think it'd be more clear to explicitly list the tasks to be executed in GitHub CI.

Let's run check on CI so that if some new tests (or other checks) are added (like with DGPv2 testFunctional), they are run by default. If we will make test dependsOn testFunctional, we will have the same issue as with current test task in other subprojects in IDEA I described above. Also, we will not solve initial issue in this case.

adam-enko commented 2 months ago

This would help with test reporting, as the tests would be reported as 'skipped' in places like Build Scan and TeamCity.

just curious: this is needed to understand if the test is skipped or just not run? I can add this tag, but adding it to every test in integration tests feels not right :)

Yeah, fair enough. I think adding a @SlowTest JUnit tag would be useful, but maybe later. I was more thinking-out-loud than actually suggesting.

I'm working on fixing the failing Windows tests in https://github.com/Kotlin/dokka/pull/3811

whyoleg commented 1 month ago

@adam-enko There is still one more test failure on Windows. Do you think may be it will be better to merge this PR and then fix the test, so that the fix will be properly tested on CI?

adam-enko commented 1 month ago

@adam-enko There is still one more test failure on Windows. Do you think may be it will be better to merge this PR and then fix the test, so that the fix will be properly tested on CI?

It's a strange failure, I'm really not sure what's wrong.

Let's suppress the test in this PR and fix it later.