SIG: Egress worker test #2944

sustrik · 2019-07-31T08:59:18Z

This change is

scrye

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @sustrik)

go/sig/egress/session/session.go, line 103 at r1 (raw file):

	go func() {
		defer log.LogPanicAndExit()
		worker.NewWorker(s, s.conn, false, s.Logger).Run()

Binding to s.conn at the start introduces a subtle side-effect. Changes in a session's underlying connection (such changes are allowed by the session.Session interface) are no longer seen by the egress code. See my comment below for more information.

go/sig/egress/worker/worker.go, line 69 at r1 (raw file):

	sess          egress.Session
	writer        SCIONWriter
	ignoreAddress bool

Add a docstring mentioning this is for test support for legacy code, and that implementations should always have it set to false.

I would also move it to the bottom of the struct, since it's not really related to the normal operation of the object.

go/sig/egress/worker/worker.go, line 79 at r1 (raw file):

}

func NewWorker(sess egress.Session, writer SCIONWriter, ignoreAddress bool,

Add a docstring stating that ignoreAddress should be false for implementations and true only for tests.

go/sig/egress/worker/worker.go, line 203 at r1 (raw file):

		w.seq = 0
	}
	bytesWritten, err := w.writer.WriteToSCION(f.raw(), snetAddr)

This can no longer take into account a Conn change in session internals, i.e., two different calls to sess.Conn() might yield different connection.

I don't know if sess.Conn() ever changes, but now it must not. Therefore, I would add it to the godoc of egress.Session.Conn that it must return the same value over the lifetime of the object.

go/sig/egress/worker/worker_test.go, line 1 at r1 (raw file):

// Copyright 2019 Anapaya Systems

Add a TestMain function to black hole logging s.t. it no longer clobbers normal test output.

Example:

scion/go/lib/infra/messenger/messenger_test.go

Lines 24 to 27 in 0e7e10e

    
           func TestMain(m *testing.M) { 
        
           	log.Root().SetHandler(log.DiscardHandler()) 
        
           	os.Exit(m.Run()) 
        
           }

go/sig/egress/worker/worker_test.go, line 41 at r1 (raw file):

}

type MockSession struct {

This mock implementation is pretty big; use gomock instead.

There are many examples of gomock usage in our codebase. Also, see https://github.com/scionproto/scion/wiki/FAQ:-Bazel-for-Go-devs#how-do-i-add-a-mock for details.

go/sig/egress/worker/worker_test.go, line 108 at r1 (raw file):

}

func (self *MockWriter) AssertFrame(t *testing.T, expected []byte) {

Once this migrates to gomock, you can use a custom-definedgomock matcher directly within the WriteToSCION expected call, which will get rid of the manual state management in the current mock WriteToSCION/AssertFrame implementations.

The nice thing about managing less ring buffer/buffer state in this test via gomock is that when application code gets refactored, the test stays the same, because it consists only of checking what should go on the network.

go/sig/egress/worker/worker_test.go, line 126 at r1 (raw file):

	}()

	// Simple packet.

The shared state in these tests means that if one breaks the state of the session framer, it will corrupt all future tests.

They should be made independent using Go subtests. Each subtest should consist of the init at the start of this function, and the cleanup at the end. Something like:

func TestParsing(t *testing.T) {
    t.Run("simple packet", func() {
        startTestWorker()
        sesssion.SendPacket()
        writer.AssertFrame()
        stopTestWorker()
   })
   t.Run("two packets single frame", func() {
       // etc
   })
}

(also, you can use a table-driven approach to get rid of repeated setup/teardown calls, if you prefer)

sustrik

Reviewable status: 2 of 11 files reviewed, 8 unresolved discussions (waiting on @scrye)

go/sig/egress/session/session.go, line 103 at r1 (raw file):