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

Jump to bottom

internal/zstd: the test may be skipped on macos systems with zstd installed. #64000

Closed
aimuz opened this issue Nov 8, 2023 · 4 comments
Closed

internal/zstd: the test may be skipped on macos systems with zstd installed. #64000

aimuz opened this issue Nov 8, 2023 · 4 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.22

Comments

@aimuz
Copy link
Contributor

aimuz commented Nov 8, 2023

What version of Go are you using (go version)?

$ go version
go version devel go1.22-c44cd69a0e Wed Nov 1 21:06:08 2023 +0800 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
irrelevant

What did you do?

On my current system I installed zstd via brew install zstd.

go test -v | grep SKIP                                                
--- SKIP: TestLarge (0.01s)
--- SKIP: TestAlloc (0.00s)
--- SKIP: FuzzDecompressor (0.00s)
--- SKIP: FuzzReverse (0.00s)
--- SKIP: FuzzXXHash (0.00s)

The reason for this is that the test cases are always looked for in /usr/bin/zstd,
whereas brew is installed in the /usr/local/bin/zstd

What did you expect to see?

Tests are not skipped

What did you see instead?

--- SKIP: TestLarge (0.01s)
--- SKIP: TestAlloc (0.00s)
--- SKIP: FuzzDecompressor (0.00s)
--- SKIP: FuzzReverse (0.00s)
--- SKIP: FuzzXXHash (0.00s)
@aimuz aimuz mentioned this issue Nov 8, 2023
Closed
aimuz added a commit to aimuz/go that referenced this issue Nov 8, 2023
@aimuz
internal/zstd: abstract zstd binary path lookup in tests
e81966c 
This commit introduces a function findZstd which is responsible for locating the
zstd executable used by fuzz and unit tests. Previously, the explicit path
"/usr/bin/zstd" was hard-coded into test cases, which could lead to skipped tests
on systems where zstd is located elsewhere or not present at that specific path.

Fixes golang#64000
aimuz added a commit to aimuz/go that referenced this issue Nov 8, 2023
@aimuz
internal/zstd: abstract zstd binary path lookup in tests
c4dfe11 
This commit introduces a function findZstd which is responsible for locating the
zstd executable used by fuzz and unit tests. Previously, the explicit path
"/usr/bin/zstd" was hard-coded into test cases, which could lead to skipped tests
on systems where zstd is located elsewhere or not present at that specific path.

Fixes golang#64000
@gopherbot
Copy link

Change https://go.dev/cl/540522 mentions this issue: internal/zstd: abstract zstd binary path lookup in tests

@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. labels Nov 8, 2023
@bcmills bcmills added this to the Go1.22 milestone Nov 8, 2023
@bcmills
Copy link
Contributor

bcmills commented Nov 8, 2023

(CC @ianlancetaylor @klauspost)

@callthingsoff
Copy link
Contributor

Also found "/usr/bin/xxhsum", should it be modified as well?

@callthingsoff callthingsoff mentioned this issue Nov 8, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/539938 mentions this issue: internal/zstd: use dynamic path resolution for xxhsum in FuzzXXHash

gopherbot pushed a commit that referenced this issue Nov 9, 2023
@callthingsoff @heschi
internal/zstd: use dynamic path resolution for xxhsum in FuzzXXHash
6b818b0 
Updates #64000

Change-Id: I71fb80128d7e2a1f82322cbf04f74db01dcc631b
GitHub-Last-Rev: 7413594
GitHub-Pull-Request: #64003
Reviewed-on: https://go-review.googlesource.com/c/go/+/539938
Run-TryBot: Jes Cok <xigua67damn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.22
Development

Successfully merging a pull request may close this issue.

internal/zstd: use dynamic path resolution for zstd in tests aimuz/go
4 participants
@bcmills @callthingsoff @gopherbot @aimuz