Skip to content

Conversation

@arielshaqed
Copy link
Contributor

@arielshaqed arielshaqed commented Nov 25, 2025

You can see results (1 measurement for now) here.

@arielshaqed arielshaqed added performance exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached area/ci labels Nov 25, 2025
@arielshaqed arielshaqed force-pushed the feature/bencher-track-test-runtime branch 6 times, most recently from 400dd88 to bff67cb Compare November 26, 2025 08:49
@arielshaqed arielshaqed requested review from a team and AliRamberg November 26, 2025 09:37
@arielshaqed arielshaqed marked this pull request as ready for review November 26, 2025 09:39
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

Very cool

  1. if we trust running the tool I don't see an issue calling the action that set it up or the curl install instead of pulling manully from github.
  2. can we use 'bencher run' without the file to and and report 'make test' step - the current code populate the github token in the same env (if we worried about passing it to untrusted process)

name: Run Go tests
runs-on: ubuntu-22.04
env:
BENCHER_API_TOKEN: ${{secrets.BENCHER_API_TOKEN}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to keep the same style

Suggested change
BENCHER_API_TOKEN: ${{secrets.BENCHER_API_TOKEN}}
BENCHER_API_TOKEN: ${{ secrets.BENCHER_API_TOKEN }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

It's only a few lines, and can be _very_ helpful to understand & debug failures
Copy link
Contributor Author

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks! Trying out the action; PTAL.

name: Run Go tests
runs-on: ubuntu-22.04
env:
BENCHER_API_TOKEN: ${{secrets.BENCHER_API_TOKEN}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@arielshaqed arielshaqed force-pushed the feature/bencher-track-test-runtime branch from bff67cb to ee4a7a2 Compare November 27, 2025 10:56
@arielshaqed arielshaqed requested a review from nopcoder November 27, 2025 10:57
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

Just making sure my comments were seen:

  • if we trust running the tool I don't see an issue calling the action that set it up or the curl install instead of pulling manully from github.
  • can we use 'bencher run' without the file to and and report 'make test' step - the current code populate the github token in the same env (if we worried about passing it to untrusted process)

the code looks the same as the initial review.

@arielshaqed arielshaqed force-pushed the feature/bencher-track-test-runtime branch from ee4a7a2 to a623471 Compare November 27, 2025 12:19
Both are about as risky, but by using the action we avoid explicitly adding
our GH token to the environment.
@arielshaqed arielshaqed force-pushed the feature/bencher-track-test-runtime branch from a623471 to 39d21f9 Compare November 27, 2025 13:17
Copy link
Contributor

@AliRamberg AliRamberg left a comment

Choose a reason for hiding this comment

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

I like this tool already! Maybe we need to measure it differently though, maybe more specific than the real/sys/user measures of the whole tests?

After reading some pages from the docs, there are really cool ways to integrate it in our CI
For tests there's actually an integration with go test -bench.
Not sure if it times the whole testing suite or each test independently but worth checking.

There're also build time and file size metrics we could collect. Probably around the goreleaser workflow...

@arielshaqed
Copy link
Contributor Author

@nopcoder seen and accepted :-). Not sure why you saw nothing, but verified that everything is now visible on the PR.

@AliRamberg absolutely we can collect more statistics. Let's start with these, please! I want to start collecting data, and add more as we go along.

Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

would prefer to reduce the complexity of the go test run just because we like to report the take under test-go-runtime.
not blocking, but if we plan to keep it for long time - or apply it in more places - we should switch to use '--build-time' with json adapter to capture the time it takes to 'make test' to compelte

@arielshaqed arielshaqed force-pushed the feature/bencher-track-test-runtime branch 2 times, most recently from 2b233d5 to 4a6fbc3 Compare November 27, 2025 16:37
- It _is_ a time to run a build-ish step.
- Better integration in bencher, shorter more understandable shell script.
- bencher will include a nice name in the report.
@arielshaqed arielshaqed force-pushed the feature/bencher-track-test-runtime branch from 4a6fbc3 to e74a22e Compare November 27, 2025 16:41
@arielshaqed
Copy link
Contributor Author

I tried to use this but we end up measuring how long it takes to download all the modules:

go list -f '{{.Dir}}/...' -m | xargs /opt/hostedtoolcache/go/1.24.10/x64/bin/go test -count=1 -coverprofile=cover.out -race -cover -failfast -parallel="4" ./...
go: downloading github.com/jamiealquiza/tachymeter v2.0.0+incompatible
go: downloading github.com/go-co-op/gocron v1.35.2
go: downloading github.com/jedib0t/go-pretty/v6 v6.6.7
go: downloading github.com/schollz/progressbar/v3 v3.13.1
go: downloading github.com/manifoldco/promptui v0.9.0
go: downloading github.com/stretchr/testify v1.11.1
[...]

I will split the test-go target into test-go-build and test-go-run, so we can measure only the second. Obviously CI users care about both, but equally obviously we care mostly about time to run tests.

@arielshaqed
Copy link
Contributor Author

I tried to use this but we end up measuring how long it takes to download all the modules:

go list -f '{{.Dir}}/...' -m | xargs /opt/hostedtoolcache/go/1.24.10/x64/bin/go test -count=1 -coverprofile=cover.out -race -cover -failfast -parallel="4" ./...
go: downloading github.com/jamiealquiza/tachymeter v2.0.0+incompatible
go: downloading github.com/go-co-op/gocron v1.35.2
go: downloading github.com/jedib0t/go-pretty/v6 v6.6.7
go: downloading github.com/schollz/progressbar/v3 v3.13.1
go: downloading github.com/manifoldco/promptui v0.9.0
go: downloading github.com/stretchr/testify v1.11.1
[...]

I will split the test-go target into test-go-build and test-go-run, so we can measure only the second. Obviously CI users care about both, but equally obviously we care mostly about time to run tests.

Computer (and golang committers) say nope. It is not possible to create a single binary running tests from >1 package. So I'm giving up on this option, and pulling. We will have some numbers: wallclock for how long test-go takes.
And we can improve it later.

@arielshaqed
Copy link
Contributor Author

THANKS for the great reviews!

@arielshaqed arielshaqed merged commit 92fdc8b into master Nov 27, 2025
40 checks passed
@arielshaqed arielshaqed deleted the feature/bencher-track-test-runtime branch November 27, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants