m-lab / traceroute-caller

A sidecar service which runs traceroute after a connection closes
Apache License 2.0
18 stars 5 forks source link

implement cached test #33

Closed yachang closed 4 years ago

yachang commented 4 years ago

This change is Reviewable

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 102


Changes Missing Coverage Covered Lines Changed/Added Lines %
ipcache/ipcache.go 38 39 97.44%
scamper/scamper.go 36 42 85.71%
<!-- Total: 78 85 91.76% -->
Totals Coverage Status
Change from base Build 79: -1.7%
Covered Lines: 397
Relevant Lines: 404

💛 - Coveralls
pboothe commented 4 years ago

ipcache/ipcache.go, line 49 at r3 (raw file):

      rc.mu.Unlock()

      rc.cache[ip].data = sc.Trace(conn, rc.cache[ip].timeStamp)

You are accessing the map outside of the mutex lock.

pboothe commented 4 years ago

scamper/scamper.go, line 168 at r3 (raw file):

}

func ExtractUUID(metaline string) string {

Now you can json.Unmarshall into a Metadata struct instead of casting a map!

pboothe commented 4 years ago

ipcache/ipcache.go, line 49 at r3 (raw file):

Previously, yachang wrote…
Done.

Please do the following:

c = &CacheTest{
  timeStamp: time.Now(),
  done:      make(chan struct{}),
}
rc.cache[ip] = c
rc.mu.Unlock()

ct.data := sc.Trace(conn, c.timeStamp)
close(c.done)
pboothe commented 4 years ago

connectionpoller/connectionpoller_test.go, line 137 at r4 (raw file):

  connPoller := New(cache)
  var tt testTracer
  connPoller.TraceClosedConnections(&tt)

You should be able to verify that it calls tt.Trace for a few connections by giving the connPoller a fake finder

pboothe commented 4 years ago

scamper/scamper.go, line 168 at r3 (raw file):

Previously, pboothe (Peter Boothe) wrote…
Now you can json.Unmarshall into a Metadata struct instead of casting a map!

Please do something like:

var metaResult Metadata
err := json.Unmarshal([]byte(metaline), &metaResult)
if err != nil {
  log.Println("Could not parse cached results:", metaline)
  return ""
}
return metaResult.UUID

It saves lines and is more typesafe

pboothe commented 4 years ago

scamper/scamper.go, line 186 at r4 (raw file):


  // remove the first line of cachedTest
  split := strings.Index(cachedTest, "\n")

Please verify that this is not -1.