Skip to content
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.

Sign up for GitHub

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

Increase the timeout period of CI to solve the unit test timeout problem of some libraries #220

Closed
wants to merge 1 commit into from
Closed

Increase the timeout period of CI to solve the unit test timeout problem of some libraries #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test-all-metadata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
test-all-metadata:
name: "🧪 ${{ matrix.coordinates }} (GraalVM ${{ matrix.versions.graalvm }} ${{ matrix.versions.java }} @ ${{ matrix.os }})"
runs-on: ${{ matrix.os }}
timeout-minutes: 20
timeout-minutes: 80
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to increase timeout by 4x? How long does it take to build #163 (comment) on GitHub Actions? I'd prefer if we could selectively bump the timeout for metadata that requires it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Completing nativeTest for org.apache.curator:curator-recipes:5.4.0 took 62 minutes on my device. I've created the smallest possible subset of unit tests for this dependency on my local device, but the full test still takes so long.

I'd prefer if we could selectively bump the timeout for metadata that requires it.

  • I don't quite understand, how to do this?

Copy link
Member

Choose a reason for hiding this comment

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

Completing nativeTest for org.apache.curator:curator-recipes:5.4.0 took 62 minutes on my device.

That is quite a lot. Were these 64min dominated by Native Image builds or by actually running the tests?

I don't quite understand, how to do this?

Right now, we don't have a mechanism for this, but maybe we can come up with something that allows overriding the timeout if necessary. We could implement that in org.graalvm.internal.tck-harness.gradle somehow, and then use:

timeout-minutes: ${{ matrix.timeout-minutes }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is quite a lot. Were these 64min dominated by Native Image builds or by actually running the tests?

  • When executing ./gradlew clean nativeTest, this task first executes the unit test of curator-recipes in a non-Native Image environment, and this process takes 29 minutes. Then the process of packaging into GraalVM Native Image took only 3 minutes, and finally executing unit tests in Native Image took 29 minutes. For reference, even directly compiling the curator-recipes component of the master branch of Apache Curator takes nearly 40 minutes.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to run the unit tests of curator-recipes on the JVM or can we skip this step?

Copy link
Contributor Author

@linghengqian linghengqian Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, this feature requires the execution of the tests in the classic "JVM" mode prior to the execution of tests in native mode.
  • I honestly don't see an option to disable nativeTest task from executing unit tests on the JVM, is there something I'm overlooking here?

Copy link
Member

Choose a reason for hiding this comment

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

I honestly don't see an option to disable nativeTest task from executing unit tests on the JVM, is there something I'm overlooking here?

You are right, I'm afraid tests currently need to run on the JVM so that we can reliably detect all test ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • So should I close this PR now?

Copy link
Member

Choose a reason for hiding this comment

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

You can unless you want to look into making timeouts adjustable per metadata entry, which I believe we will need to support curator-recipes.

needs: get-all-metadata
strategy:
fail-fast: false
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-changed-metadata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ jobs:
name: "🧪 ${{ matrix.coordinates }} (GraalVM ${{ matrix.versions.graalvm }} ${{ matrix.versions.java }} @ ${{ matrix.os }})"
if: needs.get-changed-metadata.result == 'success' && needs.get-changed-metadata.outputs.none-found != 'true'
runs-on: ${{ matrix.os }}
timeout-minutes: 20
timeout-minutes: 80
needs: get-changed-metadata

strategy:
Expand Down