- Notifications
You must be signed in to change notification settings - Fork1.1k
fix: makemake test runnable again#21251
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
DevelopmentCats left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🔍 Code Review
Reviewed memory optimization changes across test infrastructure.
Found 1 issue (1 important).
AI review viaCoder Tasks
| # Make sure to keep this in sync with test-go-race from .github/workflows/ci.yaml. | ||
| test-race: | ||
| $(GIT_FLAGS) gotestsum --junitfile="gotests.xml" -- -race -count=1 -parallel 4 -p 4 ./... | ||
| $(GIT_FLAGS) gotestsum --junitfile="gotests.xml" -- -tags=testsmallbuffers -race -count=1 -parallel 4 -p 4 ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🟡IMPORTANT: Missing-timeout flag in test-race target.
Thetest-race target runs race detection tests but doesn't specify a timeout. Race detection significantly slows down test execution (typically 2-10x slower), and without a timeout, tests could hang indefinitely in CI or local development.
Other test targets properly include timeouts:
test-postgres: uses-timeout=20m(line 1086)test-tailnet-integration: uses-timeout=5m(line 1170)
The comment on line 1156 also mentions keeping this in sync with.github/workflows/ci.yaml, which has atimeout-minutes: 25 set for thetest-go-race-pg job.
| $(GIT_FLAGS) gotestsum --junitfile="gotests.xml" -- -tags=testsmallbuffers -race -count=1 -parallel 4 -p 4 ./... | |
| $(GIT_FLAGS) gotestsum --junitfile="gotests.xml" -- -tags=testsmallbuffers -race -count=1 -parallel 4 -p 4-timeout=20m./... |
Uh oh!
There was an error while loading.Please reload this page.
All credit for this fix goes to Opus 4.5 ❤️
I identified a problem this morning; wethought the reason for tests were using up to 25GiB of RAM due to excessive postgres databases being created after we moved away from dbmem. As it turns out, about half of the in-use heap is caused by Wireguard device buffers.
coder/wireguard-go#4 is a vibed solution to this problem whereby the buffers are tunedway down when a build tag is used. Disclaimer: I have no idea what impact this has as I haven't looked into this very carefully yet, BUT the tests all pass so 🤷.
Uncached:
DONE 13293 tests, 51 skipped in 162.041sand uses about 4GiB of RAMCached:
DONE 13293 tests, 51 skipped in 7.712sand uses about 1GiB of RAMThe RAM usage observations were purely just eyeballing using
topand comparing against the baseline; confounding factors abound, but it's much better than it was at least.Needs validation.