google / pprof

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

internal/driver: return an error on failed source fetching #865

Closed odeke-em closed 6 months ago

odeke-em commented 6 months ago

This change inverts checks to ensure that we return a wrapped fs.ErrNotExist error if we completely fail to find a source fetcher, instead of panicking.

Fixes #863

odeke-em commented 6 months ago

Kind /cc @aalexand @ghemawat

aalexand commented 6 months ago

I sent https://github.com/google/pprof/pull/864 earlier today.

odeke-em commented 6 months ago

Oh cool, thanks @aalexand! Could we perhaps copy over and adapt the isolated test that I've crafted to prevent regressions?

func TestFetchDoesNotPanicOnMissingSource(t *testing.T) {
    _, _, err := fetch("non-existent-source", 10*time.Millisecond, 10*time.Millisecond, nil, http.DefaultTransport)
    if err == nil {
        t.Fatal("expected a non-nil error")
    }
    if g, w := err, fs.ErrNotExist; !errors.Is(g, w) {
        t.Fatalf("mismatched type: got %T, want %T", g, w)
    }
}
aalexand commented 6 months ago

Oh cool, thanks @aalexand! Could we perhaps copy over and adapt the isolated test that I've crafted to prevent regressions?

func TestFetchDoesNotPanicOnMissingSource(t *testing.T) {
  _, _, err := fetch("non-existent-source", 10*time.Millisecond, 10*time.Millisecond, nil, http.DefaultTransport)
  if err == nil {
      t.Fatal("expected a non-nil error")
  }
  if g, w := err, fs.ErrNotExist; !errors.Is(g, w) {
      t.Fatalf("mismatched type: got %T, want %T", g, w)
  }
}

My PR added a test already.

odeke-em commented 6 months ago

Alright, thanks!