-
Notifications
You must be signed in to change notification settings - Fork 0
fix(go-runner): ensure excludes dont filter files #47
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
fix(go-runner): ensure excludes dont filter files #47
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 a bug where files containing excluded directory names (like target.go or node_modules.go) were incorrectly being filtered out during directory copying. The root cause was the dircpy library's use of .contains() for matching exclusion patterns, which led to false positives. The fix removes the hardcoded exclusion list, allowing all files to be copied correctly.
Key Changes:
- Removed hardcoded excludes
["node_modules", "target"]from thecopy_dir_recursivelyfunction - Added comprehensive test project
example-with-excluded-nameswith files namedtarget.goandnode_modules.goto validate the fix - Added integration and discovery test cases for the new test project
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
go-runner/src/utils.rs |
Removed the hardcoded exclusion patterns that were causing false positives |
go-runner/testdata/projects/example-with-excluded-names/main_test.go |
Added benchmark test that depends on code in files with problematic names |
go-runner/testdata/projects/example-with-excluded-names/main.go |
Added main source file that imports from internal tools package |
go-runner/testdata/projects/example-with-excluded-names/internal/tools/target_impl.go |
Implementation file containing functions used by target.go and node_modules.go |
go-runner/testdata/projects/example-with-excluded-names/internal/tools/target.go |
Test file with "target" in filename to validate it's no longer excluded |
go-runner/testdata/projects/example-with-excluded-names/internal/tools/node_modules.go |
Test file with "node_modules" in filename to validate it's no longer excluded |
go-runner/testdata/projects/example-with-excluded-names/internal/tools/instrumentation.go |
Integration point that uses functions from both target and node_modules files |
go-runner/testdata/projects/example-with-excluded-names/go.mod |
Go module definition for test project |
go-runner/src/snapshots/codspeed_go_runner__integration_tests__assert_results_snapshots@example-with-excluded-names.snap |
Expected benchmark results snapshot |
go-runner/src/integration_tests.rs |
Added test case for new example project |
go-runner/src/builder/snapshots/codspeed_go_runner__builder__discovery__tests__discover_benchmarks@example-with-excluded-names.snap |
Expected discovery results snapshot |
go-runner/src/builder/discovery.rs |
Added test case for benchmark discovery validation |
The changes are well-structured and provide comprehensive test coverage for the bug fix. No issues were identified during the review.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this PR will improve performance by 22.14%Summary
Performance Changes
Comparing |
1eeef66 to
6af8732
Compare
This ensures that we always patch the logr and stdr package correclty, since they also depend on each other. We must not just update the imports, we also must patch them like we do in `patcher.rs`
6af8732 to
17de271
Compare
…iles in external test packages
72f37d8 to
f092d36
Compare
8867816 to
ca2720a
Compare
ca2720a to
5a11ae4
Compare
|
@not-matthias this should be closed or rebased right ? Feel free to re-request review |
Yep, we don't need this PR anymore as it's been superseded by #49 |
Lots of code added in this PR, but most of it is just forks of packages used by opentelemetry. I recommend going through for each commit.
Changes in this PR:
.containswhich also deletes any file that includes the exclude (e.g.target.go): https://github.com/woelper/dircpy/blob/645889b819dc78deade6d7ef16aba0420b8e5d08/src/lib.rs#L216-L223ForTestfield for external tests, which includes additional test files we have to rename (and added a new test case to validate that it works)testify(we already supported asserts, but it didn't work with thesuitepackage)Validated it on the opentelemetry-go project; https://github.com/AvalancheHQ/opentelemetry-go/actions/runs/20787115007 / https://codspeed.io/AvalancheHQ/opentelemetry-go/runs/695e81a4cc5b8dc1af806e9b (fails on CodSpeed because it has 1k benchmarks, but the run went through without any errors)