Skip to content

add test for Mac OS and Windows #1934

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger requested a review from a team as a code owner June 2, 2025 14:42
@zeitlinger
Copy link
Member Author

@trask can you help with the windows error?

@trask
Copy link
Member

trask commented Jun 3, 2025

@trask can you help with the windows error?

I think you need to add double quotes below

"-Porg.gradle.java.installations.paths=${{ steps.setup-test-java.outputs.path }}"

import org.junit.jupiter.api.io.TempDir;

@EnabledOnOs(LINUX) // Uses async-profiler, which is only supported on Linux
Copy link
Contributor

Choose a reason for hiding this comment

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

async profiler works on mac, see https://github.com/async-profiler/async-profiler

Comment on lines 35 to 36
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

was this needed?

Suggested change
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

interesting, the problem is that secrets aren't available on PR builds, so I don't think this will help (we have similar problem on link checking workflows)

Copy link
Member Author

Choose a reason for hiding this comment

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

it did fix this PR...

Copy link
Member

Choose a reason for hiding this comment

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

can you explain?

Copy link
Member

Choose a reason for hiding this comment

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

looks like you're right: https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication

let me do some testing, I'm not sure why this didn't help with the link check workflows...

Copy link
Member

Choose a reason for hiding this comment

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

finally got to the bottom of why I didn't think it worked before (was because of a bug) 😅: open-telemetry/semantic-conventions#2352 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

great - is this PR ready then?

Copy link
Member Author

Choose a reason for hiding this comment

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

@trask can you take a look?

Copy link
Member

Choose a reason for hiding this comment

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

I'm still not following how hitting only a single url would require GH_TOKEN, it's not like link checking where we are hitting 100s or 1000s of urls, so I reverted this part to investigate, but not seeing the failure that you were seeing earlier

Comment on lines +96 to +97
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

same

Suggested change
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Comment on lines +59 to +60
coverage: true
jmh-based-tests: true
Copy link
Member

Choose a reason for hiding this comment

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

are these being used?

steps:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

- id: setup-test-java
name: Set up JDK ${{ matrix.test-java-version }} for running tests
- id: setup-java-test
Copy link
Member

@trask trask Jun 18, 2025

Choose a reason for hiding this comment

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

id is renamed here but not its usage below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants