search

google / pprof

pprof is a tool for visualization and analysis of profiling data
Apache License 2.0
8.03k stars 608 forks source link

Remove pprof->chromedp dependency. #866

Closed ghemawat closed 6 months ago

ghemawat commented 6 months ago

Moved chromedp based tests to a separate module (in the same Git repository). This change signficantly reduces the dependency footprint of pprof.

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.92%. Comparing base (0ed6a68) to head (65219d9). Report is 22 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

liggitt commented 6 months ago

lgtm, thanks for carving this out

aalexand commented 6 months ago

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 testing directory and put browsertests and fuzz directories there.

ghemawat commented 6 months ago

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?

aalexand commented 6 months ago

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.