Skip to content

Conversation

@not-matthias
Copy link
Member

@not-matthias not-matthias commented Dec 17, 2025

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:

  • Turns out that the exclude filters are just checking with .contains which also deletes any file that includes the exclude (e.g. target.go): https://github.com/woelper/dircpy/blob/645889b819dc78deade6d7ef16aba0420b8e5d08/src/lib.rs#L216-L223
  • Forked stdr and logr
  • Handle the ForTest field for external tests, which includes additional test files we have to rename (and added a new test case to validate that it works)
  • Forked testify (we already supported asserts, but it didn't work with the suite package)
  • Shard the integration tests to avoid running out of disk space

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)

Copy link

Copilot AI left a 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 the copy_dir_recursively function
  • Added comprehensive test project example-with-excluded-names with files named target.go and node_modules.go to 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.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 17, 2025

Merging this PR will improve performance by 22.14%

Summary

⚡ 1 improved benchmark
✅ 48 untouched benchmarks
🆕 3 new benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
bench_collect_results[10000000, 25] 6.1 s 5 s +22.14%
🆕 BenchmarkStdrWithName N/A 5.4 µs N/A
🆕 BenchmarkStdr N/A 11.4 µs N/A
🆕 BenchmarkLogrTestr N/A 28 µs N/A

Comparing cod-1829-cannot-compile-certain-internal-packages (5a11ae4) with main (4df4d46)

Open in CodSpeed

@not-matthias not-matthias marked this pull request as draft December 17, 2025 19:01
@not-matthias not-matthias force-pushed the cod-1829-cannot-compile-certain-internal-packages branch 2 times, most recently from 1eeef66 to 6af8732 Compare January 7, 2026 11:09
@not-matthias not-matthias marked this pull request as ready for review January 7, 2026 11:10
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`
@not-matthias not-matthias force-pushed the cod-1829-cannot-compile-certain-internal-packages branch from 6af8732 to 17de271 Compare January 7, 2026 11:46
@not-matthias not-matthias force-pushed the cod-1829-cannot-compile-certain-internal-packages branch from 72f37d8 to f092d36 Compare January 7, 2026 15:04
@not-matthias not-matthias force-pushed the cod-1829-cannot-compile-certain-internal-packages branch 2 times, most recently from 8867816 to ca2720a Compare January 7, 2026 16:45
@not-matthias not-matthias force-pushed the cod-1829-cannot-compile-certain-internal-packages branch from ca2720a to 5a11ae4 Compare January 7, 2026 18:09
@GuillaumeLagrange
Copy link

@not-matthias this should be closed or rebased right ? Feel free to re-request review

@not-matthias
Copy link
Member Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants