Remove pprof->chromedp dependency. #866
Conversation
Moved chromedp based tests to a separate module (in the same Git repository). This change signficantly reduces the dependency footprint of pprof.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
==========================================
+ Coverage 66.86% 66.92% +0.05%
==========================================
Files 44 44
Lines 9824 9793 -31
==========================================
- Hits 6569 6554 -15
+ Misses 2794 2784 -10
+ Partials 461 455 -6 ☔ View full report in Codecov by Sentry. |
browsertests/go.mod
Outdated
|
|
||
| require ( | ||
| github.com/chromedp/chromedp v0.9.2 | ||
| github.com/google/pprof v0.0.0-20240521024322-9665fa269a30 |
There was a problem hiding this comment.
you'll want this to exercise the pprof code at HEAD, not this locked version
to do that, point this module at the local pprof module with something like this:
replace github.com/google/pprof => ../
and probably switch the require directive to github.com/google/pprof v0.0.0 to make it clear this is not pinning to a specific pprof version
There was a problem hiding this comment.
Great catch. Will do.
| @@ -12,7 +12,7 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
There was a problem hiding this comment.
can this be kept inside the internal package, possibly within internal/driver/browsertests or something similar?
There was a problem hiding this comment.
I was matching the placement of fuzz.
Plus internal/ means something special to Go, so placing a module crossing boundary inside an internal/... subtree may cause some confusion.
There was a problem hiding this comment.
Plus internal/ means something special to Go, so placing a module crossing boundary inside an internal/... subtree may cause some confusion.
It does function properly to refer up the tree from an internal/... module (experiment at HEAD of https://github.com/liggitt/pprof/commits/chromedpdep/), and I thought putting this test-only module underneath internal would ensure nothing outside pprof could accidentally refer to it, which could reduce confusion.
But if you think it's less confusing as a visible test-only package, that's fine as well.
There was a problem hiding this comment.
If pprof/internal/browsertests is its own module, is it still "internal" in the special Go sense that it cannot be imported externally?
I suspect it's just a normal module with github.com/google/pprof/internal/browsertest as import path. That's sending a message to potential consumers that they shouldn't import, which is useful, but it's not as strict as an internal package.
| github.com/chzyer/readline v1.5.1 | ||
| github.com/ianlancetaylor/demangle v0.0.0-20240312041847-bd984b5ce465 | ||
| ) | ||
|
|
||
| require ( |
There was a problem hiding this comment.
this reduction in transitive deps for downstreams is exactly what I was hoping to see 🎉
|
lgtm, thanks for carving this out |
test.sh
Outdated
| @@ -29,6 +29,9 @@ PKG=$(go list -f '{{if .TestGoFiles}} {{.ImportPath}} {{end}}' ./...) | |||
|
|
|||
| go test $PKG | |||
|
|
|||
| # Browser tests are in a separate module, so do not show in $PKG. | |||
| (cd browsertests && go test) | |||
There was a problem hiding this comment.
Will we lose code coverage and race detection through browser tests with this change?
Plus go vet checks and formatting. And staticcheck above.
There was a problem hiding this comment.
Good point. Added staticcheck, vet, race, lint, fmt.
Did not add coverage since browsertests only tests javascript code for which we cannot currently get any coverage info.
browsertests/testutils_test.go
Outdated
| @@ -0,0 +1,216 @@ | |||
| package browser_test | |||
There was a problem hiding this comment.
Maybe "package browsertests" to match the directory name?
Same for browser_test.go file.
browsertests/testutils_test.go
Outdated
| @@ -0,0 +1,216 @@ | |||
| package browser_test | |||
|
|
|||
There was a problem hiding this comment.
The name of this file testutils_test.go is a bit weird since this file is not a test. I think I'd name it either testutils.go or browsertestsutil.go or something like that.
|
Not merging yet since there is a pending discussion about whether we want to put these tests in internal. I'm not entirely sure I would like them in internal since I wouldn't call integration tests like this an internal thing. At the same time more top-level directories is not exciting either. One thought I had is maybe we could have top-level |
|
I don't like internal either since it does not work across module boundaries. If anything, we could if warranted, put an internal dir under browsertests to hold internal stuff. But seems unnecessary since that package exports nothing. I like the idea of putting things under a testing directory: testing/fuzz, testing/browser are good names. We will want a separate PR for moving fuzz anyway. We can move browsertests then, so maybe no reason to hold off on this PR? |
Makes sense, we can address further changes separately. |
Moved chromedp based tests to a separate module (in the same Git repository). This change signficantly reduces the dependency footprint of pprof.