Fix integration test dependencies to build and cosmetic changes #1502
Conversation
|
This looks nice, But it might introduce some undesired behaviour https://github.com/Baraa-Hasheesh/async-profiler/actions/runs/17956941491 |
.github/workflows/build.yml
Outdated
| type: string | ||
| required: true | ||
| description: "Platform identifier (linux-x64, linux-arm64, macos)" |
There was a problem hiding this comment.
What are these definitions for? Are they even visible anywhere?
There was a problem hiding this comment.
These are for the developer, not visible in UI afaik.
There was a problem hiding this comment.
For developers, you mean, for me and you? :) But this looks like some formal API description, not quite human-friendly. In other words, it's just too much boilerplate to explain trivial things.
| build-and-upload-binaries: | ||
| build-linux-arm64: | ||
| name: build / linux-arm64 | ||
| uses: ./.github/workflows/build.yml |
There was a problem hiding this comment.
Why do all paths start with ./? Doesn't it work without it?
| java-version: ${{ matrix.java-version }} | ||
| architecture: ${{ matrix.architecture || '' }} | ||
|
|
||
| integ-containers: |
There was a problem hiding this comment.
What do you mean by "containers" here?
Why is it separate from integ-linux-x64 and integ-linux-arm64 that also run in containers?
There was a problem hiding this comment.
I meant "non-default", considering public.ecr.aws/async-profiler/asprof-builder-x86 and public.ecr.aws/async-profiler/asprof-builder-arm default. Will find a better name.
There was a problem hiding this comment.
Merged in to integ-linux-x64.
There was a problem hiding this comment.
There is nothing special about these containers. The only difference is that integ-linux-* run in the same containers that are used for building artifacts, but this does not really matter. My points is, division by "default" and "non-default" is not justitied, let's keep it uniform.
There was a problem hiding this comment.
Yes, removed the "default" vs. "non-default" separation.
|
|
||
| integ-linux-arm64: | ||
| name: integration test / linux-arm64 | ||
| needs: [build-jars, build-linux-arm64] |
There was a problem hiding this comment.
Integration tests previously did not depend on build-jars? What has changed?
| container-image: public.ecr.aws/async-profiler/asprof-builder-alpine:corretto-11 | ||
| - test-platform: linux-x64-AL2 | ||
| container-image: public.ecr.aws/async-profiler/asprof-builder-amazonlinux:2 | ||
| container-volumes: '["/tmp/node20:/__e/node20"]' |
There was a problem hiding this comment.
There used to be a comment here to explain why this cryptic config is needed. Can we keep it?
async-profiler/.github/workflows/test-and-publish-nightly.yml
Lines 163 to 166 in d97a7d3
| run: "[ ! -f /root/setup.sh ] || /root/setup.sh" | ||
| - name: Setup Java | ||
| uses: actions/setup-java@v4 | ||
| if: ${{ !contains(inputs.test-platform, 'alpine') }} |
There was a problem hiding this comment.
Can we keep the comment here to explain why Alpine is excluded?
async-profiler/.github/workflows/test-and-publish-nightly.yml
Lines 186 to 187 in d97a7d3
This is a feature. No point in cancelling linux tests when macos tests are failing. Linux tests may fail or pass and that could give the developer an idea of what may be wrong. |
|
On your screenshots, previous workflow ran for 4:41, whereas the new "optimized" one ran for more than a minute longer. Why is that? |
It may have to do with waiting for a GHA runner, as I was triggering runs in quick succession before I took the screenshot. |
|
@krk Can job titles be shortened? This is unreadable: 
|
4 participants
Before this PR, all integration tests depended on all builds, causing e.g. linux-x64 integration tests to wait for macos build.
This is now fixed with some cosmetic changes to make the UI look like attached:
Previous look and structure
Motivation and context
Integration tests should not wait for builds on unrelated OS/archs.
How has this been tested?
By the current PR github action.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.