-
Notifications
You must be signed in to change notification settings - Fork 0
fix: imports for synctest #30
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
Conversation
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.
Pull Request Overview
This PR fixes import issues in the testing package fork, specifically addressing problems with the testing/synctest package that was still referencing the upstream testing package instead of the CodSpeed fork. The fix includes patching imports across multiple files and adding tests to prevent similar issues in the future.
- Updated import statements throughout the testing fork to use the CodSpeedHQ fork path
- Renamed linkname functions to avoid linker blocklists
- Added integration tests with new test files to verify synctest functionality
Reviewed Changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testing/testing/testing.go | Updated import examples and renamed linkname function to avoid blocklist |
| testing/testing/synctest/synctest.go | Fixed imports and added documentation for renamed linkname function |
| testing/testing/slogtest/slogtest.go | Updated testing import to use forked package |
| testing/patches/synctest.patch | Added new patch file for synctest modifications |
| testing/internal/testenv/testenv.go | Fixed testing import path |
| testing/internal/testenv/exec.go | Fixed testing import path |
| testing/internal/synctest/synctest.go | Added manual Escape implementation to avoid abi dependency |
| testing/internal/abi/*.go | Removed entire abi package and related files |
| testing/fork.sh | Updated script to remove abi from copied packages and apply synctest patch |
| go-runner/src/snapshots/*.snap | Updated test snapshots with new benchmark entries |
| go-runner/src/lib.rs | Added panic on build failure in test mode for early detection |
| go-runner/src/builder/mod.rs | Disabled workspace mode to avoid build flag conflicts |
| example/sub-packages/buffer_test.go | Added new test file with synctest usage examples |
| example/sub-packages/buffer.go | Added supporting code for new tests |
Comments suppressed due to low confidence (4)
testing/patches/synctest.patch:1
- Corrected spelling of 'lead' to 'led' for past tense consistency.
testing/patches/synctest.patch:1 - Corrected spelling of 'linked' to 'linker'.
testing/testing/synctest/synctest.go:1 - Corrected spelling of 'linked' to 'linker'.
testing/internal/synctest/synctest.go:1 - Corrected spelling of 'lead' to 'led' for past tense consistency.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a64f7be to
f9efbbb
Compare
CodSpeed Performance ReportMerging #30 will improve performances by 15.38%Comparing Summary
Benchmarks breakdown
|
GuillaumeLagrange
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.
olgtm
144bb82 to
d8d04cb
Compare
Forgot to properly patch the imports of the testing fork, so we had an error when we tried to include the
testing/synctestpackage because it was still using the upstreamtestingpackage.It's now fixed, and I added a few tests to ensure we catch this in the future.
EDIT: Also fixed a build error in fuego that was caused by them using workspaces. We're now panicking in tests when the build fails to catch those errors earlier