Initial go integration test framework #1694

lukedirtwalker · 2018-07-19T14:35:33Z

Contributes to #1693

This change is

worxli

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @worxli, and @kormat)

a discussion (no related file):
Could you add some documentation? Also when I run ./bin/pp_integration I get Error during client execution err signal: killed Failed to run tests signal: killed. That's not quite helpful?

lukedirtwalker

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @worxli, and @kormat)

a discussion (no related file):

Previously, worxli (Lukas Bischofberger) wrote…

Could you add some documentation? Also when I run ./bin/pp_integration I get Error during client execution err signal: killed Failed to run tests signal: killed. That's not quite helpful?

Added the README.md as discussed. Logging will be improved in a later commit.

worxli

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @worxli, and @kormat)

go/lib/integration/integration.go, line 51 at r2 (raw file):

var (
	scionPath string

That is never used?

worxli

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker, @worxli, and @kormat)

go/examples/pingpong/pp_integration/pingpong.go, line 25 at r2 (raw file):

func main() {
	os.Exit(realMain())

Can we document/(add a comment) somewhere why this needs to be done?

go/examples/pingpong/pp_integration/pingpong.go, line 30 at r2 (raw file):

err :=

This can be put on one line: if err := integration.Init(); err != nil {

lukedirtwalker

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker, @worxli, and @kormat)

go/examples/pingpong/pp_integration/pingpong.go, line 25 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Can we document/(add a comment) somewhere why this needs to be done?

Added it to the README.md

go/examples/pingpong/pp_integration/pingpong.go, line 30 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

err :=
This can be put on one line: if err := integration.Init(); err != nil {

Done.

go/lib/integration/integration.go, line 51 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

That is never used?

Yap, Removed

shitz

Reviewed 1 of 4 files at r1, 2 of 3 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @worxli, @lukedirtwalker, and @kormat)

go/examples/pingpong/pp_integration/pingpong.go, line 1 at r4 (raw file):

// Copyright 2018 ETH Zurich

Change copyright here and in all new files

go/examples/pingpong/pp_integration/pingpong.go, line 45 at r4 (raw file):

		[]string{"-mode", "client", "-sciondFromIA", "-count", "1",
			"-local", integration.LocalAddrReplace + ",[127.0.0.1]:0",
			"-remote", integration.RemoteAddrReplace + ",[127.0.0.1]:40004"},

Add a constant for the server port

go/lib/integration/binary.go, line 26 at r4 (raw file):

const (
	LocalAddrReplace  = "<LOCALADDR>"

Add a docstring to BinaryIntegration explaining that these placeholders will be replaced dynamically with IAs. Also, I'd call it <SRCIA> and <DSTIA>, since they are not fully qualified scion addresses.

go/lib/integration/binary.go, line 52 at r4 (raw file):

func (bi *BinaryIntegration) StartServer(ctx context.Context, local addr.IA) (Runner, error) {

No need for newline here

go/lib/integration/integration.go, line 48 at r4 (raw file):

type Runner interface {
	// Wait should block until the underlying program is terminated.
	Wait() error

Runner implies that there should be a Run() method. If Wait() is the only method of the interface I'd call this Waiter. Alternatively, I'd add a Run() or Start() method.

go/lib/integration/integration.go, line 57 at r4 (raw file):

}

func addTestFlags() {

I think you can call log.AddLogConsFlag() in Init() directly.

go/lib/integration/integration.go, line 76 at r4 (raw file):

// Connection is a source, destination pair. The client (Src) will dial the server (Dst).
type Connection struct {

I would call this IAPair

go/lib/integration/integration.go, line 83 at r4 (raw file):

// GenerateAllSrcDst generates the cartesian product shuffle(asList) x shuffle(asList).
func GenerateAllSrcDst(asList *util.ASList) []Connection {
	allAs := append(asList.Core, asList.NonCore...)

Maybe call this allSrcAses and alldDstAses?

go/lib/integration/integration.go, line 121 at r4 (raw file):

		}
		defer s.Wait()
		defer serverCancel()

You can move this to just after serverCtx, ... := .... Then you don't have to call serverCancel() in the if-block.

go/lib/integration/integration.go, line 148 at r4 (raw file):

}

func extraceDsts(connections []Connection) []addr.IA {

s/extrace/extract/g
I would also call this extractUniqueDsts

go/lib/integration/integration.go, line 150 at r4 (raw file):

func extraceDsts(connections []Connection) []addr.IA {
	uniqueDsts := make(map[addr.IA]bool)
	for _, endp := range connections {

The method can be simplified

seen := make(map[addr.IA]bool)
var res []addr.IA
for _, pair := range connections {
    if !seen[pair.Dst] {
        res = append(res, pair.Dst)
        seen[pair.Dst] = true 
    }
}
return res

go/lib/integration/README.md, line 23 at r4 (raw file):

* It should exit (`os.Exit()`) with 0 on success and with a non-zero value on error.
* The `Integration` interface and the methods in integration should be used to implement the test.
* An example can be found in `go/examples/pingpong/pp_integration`.

Add newline

lukedirtwalker

Reviewable status: 1 of 5 files reviewed, 13 unresolved discussions (waiting on @shitz, @worxli, @lukedirtwalker, and @kormat)

go/examples/pingpong/pp_integration/pingpong.go, line 1 at r4 (raw file):

Previously, shitz wrote…

Change copyright here and in all new files

Done.

go/examples/pingpong/pp_integration/pingpong.go, line 45 at r4 (raw file):

Previously, shitz wrote…

Add a constant for the server port

Done.

go/lib/integration/binary.go, line 26 at r4 (raw file):

Previously, shitz wrote…

Add a docstring to BinaryIntegration explaining that these placeholders will be replaced dynamically with IAs. Also, I'd call it <SRCIA> and <DSTIA>, since they are not fully qualified scion addresses.

Done.

go/lib/integration/binary.go, line 52 at r4 (raw file):

Previously, shitz wrote…

No need for newline here

Done.

go/lib/integration/integration.go, line 48 at r4 (raw file):

Previously, shitz wrote…

Runner implies that there should be a Run() method. If Wait() is the only method of the interface I'd call this Waiter. Alternatively, I'd add a Run() or Start() method.

Done.

go/lib/integration/integration.go, line 57 at r4 (raw file):

Previously, shitz wrote…

I think you can call log.AddLogConsFlag() in Init() directly.

There will be more flags in the future. So I think it might sense to keep this.

go/lib/integration/integration.go, line 76 at r4 (raw file):

Previously, shitz wrote…

I would call this IAPair

Done.

go/lib/integration/integration.go, line 83 at r4 (raw file):

Previously, shitz wrote…

Maybe call this allSrcAses and alldDstAses?

Done.

go/lib/integration/integration.go, line 121 at r4 (raw file):

Previously, shitz wrote…

You can move this to just after serverCtx, ... := .... Then you don't have to call serverCancel() in the if-block.

I don't think so, the order matters: we need to call serverCancel() first and then s.Wait(). Up there we can't make any assumptions about s yet.

go/lib/integration/integration.go, line 148 at r4 (raw file):

Previously, shitz wrote…

s/extrace/extract/g
I would also call this extractUniqueDsts

Done.

go/lib/integration/integration.go, line 150 at r4 (raw file):

Previously, shitz wrote…

The method can be simplified

seen := make(map[addr.IA]bool)
var res []addr.IA
for _, pair := range connections {
    if !seen[pair.Dst] {
        res = append(res, pair.Dst)
        seen[pair.Dst] = true 
    }
}
return res

Done.

go/lib/integration/README.md, line 23 at r4 (raw file):

Previously, shitz wrote…

Add newline

Done.

shitz

Reviewed 4 of 4 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @worxli, @shitz, @lukedirtwalker, and @kormat)

go/lib/integration/binary.go, line 35 at r5 (raw file):

// Use SrcIAReplace and DstIAReplace in arguments as placeholder for the source and destination IAs.
// When starting a client/server the placeholders will be replaced with the actual values.
type binaryIntegration struct {

You shouldn't make this type private as it will prevent the docstring from showing up in godoc. If the type is public again I would also den make the constructor return the concrete type.

go/lib/integration/integration.go, line 121 at r4 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I don't think so, the order matters: we need to call serverCancel() first and then s.Wait(). Up there we can't make any assumptions about s yet.

As discussed offline, defer those as one unit.

lukedirtwalker

Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @shitz, @worxli, @lukedirtwalker, and @kormat)

go/lib/integration/binary.go, line 35 at r5 (raw file):

Previously, shitz wrote…

You shouldn't make this type private as it will prevent the docstring from showing up in godoc. If the type is public again I would also den make the constructor return the concrete type.

Moved the comment to the constructor. The client shouldn't really care about the concrete implementation.

go/lib/integration/integration.go, line 121 at r4 (raw file):

Previously, shitz wrote…

As discussed offline, defer those as one unit.

Done.

shitz

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @worxli and @kormat)

kormat

Reviewed 1 of 4 files at r1, 2 of 3 files at r3, 1 of 2 files at r4, 2 of 4 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @worxli and @lukedirtwalker)

go/examples/pingpong/pp_integration/pingpong.go, line 41 at r4 (raw file):

	}
	// TODO(lukedirtwalker) we should enable logging
	// depeding on the main log parameter it should either go to a file or to console stderr.

"depending"

go/lib/integration/integration.go, line 15 at r4 (raw file):

// limitations under the License.

// Package integration provides function to simplify the creation of integration tests.

"Package integration simplifies the creation of integration tests."

go/lib/integration/integration.go, line 110 at r6 (raw file):

// RunTests runs the client and server for each IAPair.
// In case of an error the function is terminated immediately.
func RunTests(in Integration, pairs []IAPair) error {

This logic is integration-test-specific, and hence i don't think it should be in the library. E.g. if this was used to run the current python end2end tests, the servers would timeout and exit with an error, as it's designed to receive a single message per server lifetime.

go/lib/integration/integration.go, line 112 at r6 (raw file):

func RunTests(in Integration, pairs []IAPair) error {
	start := time.Now()

Drop blank line.

go/lib/integration/integration.go, line 128 at r6 (raw file):

		}()
	}

Drop blank line.

go/lib/integration/integration.go, line 146 at r6 (raw file):

		}
	}

Drop blank line.

go/lib/integration/integration.go, line 149 at r6 (raw file):

	elapsed := time.Since(start)
	fmt.Printf("Test %v successful, used %v\n", in.Name(), elapsed)

Drop blank line.

worxli

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker)

lukedirtwalker

Reviewable status: 3 of 5 files reviewed, 7 unresolved discussions (waiting on @kormat, @shitz, and @lukedirtwalker)

go/examples/pingpong/pp_integration/pingpong.go, line 41 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

"depending"

Done.

go/lib/integration/integration.go, line 15 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

"Package integration simplifies the creation of integration tests."

Done.

go/lib/integration/integration.go, line 110 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This logic is integration-test-specific, and hence i don't think it should be in the library. E.g. if this was used to run the current python end2end tests, the servers would timeout and exit with an error, as it's designed to receive a single message per server lifetime.

As discussed there are now more primitives to build a test. Runtests was moved to pingpong. Done.

go/lib/integration/integration.go, line 112 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Drop blank line.

Done.

go/lib/integration/integration.go, line 128 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Drop blank line.

Done.

go/lib/integration/integration.go, line 146 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Drop blank line.

Done.

go/lib/integration/integration.go, line 149 at r6 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Drop blank line.

Done.

shitz

Reviewed 2 of 2 files at r7.
Dismissed @kormat from 4 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kormat and @lukedirtwalker)

go/lib/integration/integration.go, line 79 at r7 (raw file):

// AllIAs returns all IA from asList.
func AllIAs(asList *util.ASList) []addr.IA {

This could be moved to util.ASList as a method on the type. I would also call it AllASes, since the type is called ASList and not IAList.

go/lib/integration/integration.go, line 127 at r7 (raw file):

// StartServer runs a server. The server can be stopped by calling Close() on the returned Closer.
func StartServer(in Integration, dst addr.IA) (io.Closer, error) {
	serverCtx, serverCancel := context.WithCancel(context.Background()) // add method to run a single server that returns a closer (go chanel to close).

line length

kormat

Reviewed 2 of 2 files at r7.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

go/examples/pingpong/pp_integration/pingpong.go, line 53 at r7 (raw file):

		[]string{"-mode", "server", "-sciondFromIA",
			"-local", integration.DstIAReplace + ",[127.0.0.1]:" + serverPort})
	pairs := integration.GenerateAllSrcDst(integration.AllIAs(asList), integration.AllIAs(asList))

The existing integration tests allow you to specify a src and/or dst IA on the commandline. If only a src IA is specified, the test is run from there to all IAs. It would be good to also support that. If they aren't specified, then use the full cross-product as you're doing here.

go/lib/integration/binary.go, line 61 at r7 (raw file):

		exec.CommandContext(ctx, bi.name, args...),
	}
	return r, r.Start()

The stdout/stderr for the server and client are both nil, meaning they are discarded, which makes debugging ~impossible. I think you need to start goroutines here to read and log the stdout/stderr from the subprocesses.

go/lib/integration/integration.go, line 53 at r7 (raw file):

}

// Init initializes the integration test, it adds and validates the command line flags.

and initializes logging.

go/lib/integration/integration.go, line 127 at r7 (raw file):

Previously, shitz wrote…

line length

I'm also not sure i understand the comment.

go/lib/integration/integration.go, line 127 at r7 (raw file):

// StartServer runs a server. The server can be stopped by calling Close() on the returned Closer.
func StartServer(in Integration, dst addr.IA) (io.Closer, error) {
	serverCtx, serverCancel := context.WithCancel(context.Background()) // add method to run a single server that returns a closer (go chanel to close).

What about allowing the app to pass in a context, so they can set their own deadline?

worxli

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker and @shitz)

go/examples/pingpong/pp_integration/pingpong.go, line 77 at r7 (raw file):

		for i, conn := range pairs {
			log.Info(fmt.Sprintf("Test %v: %v -> %v (%v/%v)",
				in.Name(), conn.Src, conn.Dst, i, len(pairs)))

Could that be i+1, len(pairs) ?

lukedirtwalker

Reviewable status: 2 of 6 files reviewed, 7 unresolved discussions (waiting on @kormat, @shitz, @lukedirtwalker, and @worxli)

go/examples/pingpong/pp_integration/pingpong.go, line 53 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

The existing integration tests allow you to specify a src and/or dst IA on the commandline. If only a src IA is specified, the test is run from there to all IAs. It would be good to also support that. If they aren't specified, then use the full cross-product as you're doing here.

Done.

go/examples/pingpong/pp_integration/pingpong.go, line 77 at r7 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Could that be i+1, len(pairs) ?

Done.

go/lib/integration/binary.go, line 61 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

The stdout/stderr for the server and client are both nil, meaning they are discarded, which makes debugging ~impossible. I think you need to start goroutines here to read and log the stdout/stderr from the subprocesses.

Added the logging stuff to this commit. (it wasn't that much in the end)

go/lib/integration/integration.go, line 53 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

and initializes logging.

Done.

go/lib/integration/integration.go, line 79 at r7 (raw file):

Previously, shitz wrote…

This could be moved to util.ASList as a method on the type. I would also call it AllASes, since the type is called ASList and not IAList.

Done.

go/lib/integration/integration.go, line 127 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I'm also not sure i understand the comment.

Done. This was a TODO for me. Remove the comment.

go/lib/integration/integration.go, line 127 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

What about allowing the app to pass in a context, so they can set their own deadline?

Do you have a use case where it makes sense to have a deadline on the server? Is it not enough to simply close it, once you are no longer needing it?

lukedirtwalker

Reviewable status: 2 of 6 files reviewed, 8 unresolved discussions (waiting on @kormat, @shitz, and @worxli)

go/lib/integration/binary.go, line 98 at r8 (raw file):

func redirectLog(name, pName string, ep io.ReadCloser) {
	defer ep.Close()

Missing defer log.LogPanicAndExit()

shitz

Reviewed 4 of 4 files at r8.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kormat and @worxli)

kormat

Reviewed 2 of 4 files at r8, 1 of 1 files at r9.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @kormat and @lukedirtwalker)

go/examples/pingpong/pp_integration/pingpong.go, line 1 at r9 (raw file):

// Copyright 2018 Anapaya Systems

Name this main.go.

go/examples/pingpong/pp_integration/pingpong.go, line 73 at r9 (raw file):

			log.Info(fmt.Sprintf("Test %v: %v -> %v (%v/%v)",
				in.Name(), conn.Src, conn.Dst, i+1, len(pairs)))

Drop blank line.

go/lib/integration/binary.go, line 107 at r9 (raw file):

func logLogEntry(name string, e logparse.LogEntry) {
	var logFun func(string, ...interface{})
	switch e.Level {

This looks like something that should be in go/lib/log instead.

Proposal:

Move go/lib/logparse.Lvl to go/lib/log
Add this to go/lib/log:

func Log(lvl Lvl, msg string, ctx ...interface{}) {
    var logFun func(string, ...interface{})
    switch (lvl) {
        case LvlDebug:
           logFun = Debug
    ...
    }
    logFun(msg, ctx...)
}

You can then call this here easily, with log.Log(e.Level, fmt.Sprintf("%s: %s", name, strings.join(e.Lines, "\n"))

go/lib/integration/binary.go, line 119 at r9 (raw file):

		logFun = log.Crit
	}
	indent := strings.Repeat(" ", len(name)+2)

I don't think an indent is needed, multi-line log statements are already self-indenting.

go/lib/integration/binary.go, line 120 at r9 (raw file):

	}
	indent := strings.Repeat(" ", len(name)+2)
	logFun(name + ": " + strings.Join(e.Lines, "\n"+indent))

In general we prefer using fmt.Format over string concatenation.

This should also indicate (at a minimum) the IA that the client/server was running in.

go/lib/integration/integration.go, line 127 at r7 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Do you have a use case where it makes sense to have a deadline on the server? Is it not enough to simply close it, once you are no longer needing it?

All the current python integration tests work this way - if the server doesn't receive a message within X seconds, it exits with an error.

go/lib/integration/integration.go, line 35 at r9 (raw file):

type iaArgs []addr.IA

func (a *iaArgs) String() string {

There's no need to use a pointer receiver here. Set must be a pointer receiver, due to the semantics of the Value interface, but any other methods can just be value receivers.

go/lib/integration/integration.go, line 43 at r9 (raw file):

}

func (a *iaArgs) Set(value string) error {

Add a docstring saying that this implements flag.Value.

kormat

Reviewed 1 of 4 files at r8.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lukedirtwalker)

lukedirtwalker

Reviewable status: 3 of 9 files reviewed, 8 unresolved discussions (waiting on @kormat, @shitz, and @lukedirtwalker)

go/lib/integration/binary.go, line 107 at r9 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This looks like something that should be in go/lib/log instead.

Proposal:

Move go/lib/logparse.Lvl to go/lib/log

Add this to go/lib/log:
func Log(lvl Lvl, msg string, ctx ...interface{}) {
    var logFun func(string, ...interface{})
    switch (lvl) {
        case LvlDebug:
           logFun = Debug
    ...
    }
    logFun(msg, ctx...)
}
You can then call this here easily, with log.Log(e.Level, fmt.Sprintf("%s: %s", name, strings.join(e.Lines, "\n"))

Done.

go/lib/integration/binary.go, line 119 at r9 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I don't think an indent is needed, multi-line log statements are already self-indenting.

Done.

go/lib/integration/binary.go, line 120 at r9 (raw file):

Previously, kormat (Stephen Shirley) wrote…

In general we prefer using fmt.Format over string concatenation.

This should also indicate (at a minimum) the IA that the client/server was running in.

Done.

go/lib/integration/integration.go, line 127 at r7 (raw file):

Previously, kormat (Stephen Shirley) wrote…

All the current python integration tests work this way - if the server doesn't receive a message within X seconds, it exits with an error.

hm actually the app can just call in.StartServer(ctx) it already provides this interface. This is just a helper function that doesn't require a context. Added a comment to indicate this.

go/lib/integration/integration.go, line 35 at r9 (raw file):

Previously, kormat (Stephen Shirley) wrote…

There's no need to use a pointer receiver here. Set must be a pointer receiver, due to the semantics of the Value interface, but any other methods can just be value receivers.

Done.

go/lib/integration/integration.go, line 43 at r9 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Add a docstring saying that this implements flag.Value.

Done.

go/examples/pingpong/pp_integration/pingpong.go, line 1 at r9 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Name this main.go.

Done.

go/examples/pingpong/pp_integration/pingpong.go, line 73 at r9 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Drop blank line.

Done.

kormat

Reviewed 7 of 7 files at r10.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)

go/examples/pingpong/pingpong.go, line 349 at r10 (raw file):

		LogFatal("Unable to listen", "err", err)
	}
	fmt.Printf("Listening ia=%s\n", local.IA) // Needed for integration test ready signal.

I think we should probably hide this behind an env var, to make it a bit cleaner.

if len(os.Getenv("SCION_GO_INTEGRATION")) > 0 {
   fmt.Printf("Listenening...")
}

and then set that env var when spawning the command.

go/lib/integration/binary.go, line 84 at r10 (raw file):

	go func() {
		defer log.LogPanicAndExit()
		defer sp.Close()

I don't think this is safe. The subprocess will block indefinitely if it outputs much to stdout (and then get killed). So.. we might have to just keep the fd open and keep reading (and discarding) any output.

go/lib/integration/binary.go, line 88 at r10 (raw file):

		for scanner.Scan() {
			line := scanner.Text()
			if fmt.Sprintf("%s%s", ReadySignal, dst) == line {

Don't run fmt.Sprintf with a constant value inside a loop :P

go/lib/integration/binary.go, line 140 at r10 (raw file):

}

func readyConsumer(local addr.IA) {

Unused.

lukedirtwalker

Reviewable status: 8 of 10 files reviewed, 4 unresolved discussions (waiting on @kormat)

go/examples/pingpong/pingpong.go, line 349 at r10 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I think we should probably hide this behind an env var, to make it a bit cleaner.
if len(os.Getenv("SCION_GO_INTEGRATION")) > 0 {
   fmt.Printf("Listenening...")
}
and then set that env var when spawning the command.

Done. I used integration. variables directly. Maybe we could also introduce integration.PrintServerReady() to hide the details, but then again other clients (e.g. python) also need to implement it this way, so we have this as an example.

go/lib/integration/binary.go, line 84 at r10 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I don't think this is safe. The subprocess will block indefinitely if it outputs much to stdout (and then get killed). So.. we might have to just keep the fd open and keep reading (and discarding) any output.

Done.

go/lib/integration/binary.go, line 88 at r10 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Don't run fmt.Sprintf with a constant value inside a loop :P

Done.

go/lib/integration/binary.go, line 140 at r10 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Unused.

Done.

kormat

Reviewed 2 of 2 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved

Fixes scionproto#1693

- Remove bind - Use a single method Listen / Dial - Use {s}net.UDPAddr as arguments - Improve naming of arguments Contributes scionproto#3136

- Remove bind - Use a single method Listen / Dial - Use {s}net.UDPAddr as arguments - Improve naming of arguments Contributes #3136

lukedirtwalker assigned worxli and kormat Jul 19, 2018

worxli suggested changes Jul 20, 2018

View reviewed changes

lukedirtwalker commented Jul 20, 2018

View reviewed changes

worxli suggested changes Jul 20, 2018

View reviewed changes

lukedirtwalker force-pushed the pubGoIntegrationTesting branch from cb456e6 to 817007a Compare July 20, 2018 10:37

lukedirtwalker commented Jul 20, 2018

View reviewed changes

shitz requested changes Jul 24, 2018

View reviewed changes

lukedirtwalker commented Jul 24, 2018

View reviewed changes

shitz requested changes Jul 24, 2018

View reviewed changes

lukedirtwalker commented Jul 24, 2018

View reviewed changes

shitz approved these changes Jul 24, 2018

View reviewed changes

kormat suggested changes Jul 24, 2018

View reviewed changes

worxli approved these changes Jul 25, 2018

View reviewed changes

lukedirtwalker commented Jul 25, 2018

View reviewed changes

shitz requested changes Jul 25, 2018

View reviewed changes

worxli removed their assignment Jul 26, 2018

kormat suggested changes Jul 26, 2018

View reviewed changes

worxli suggested changes Jul 26, 2018

View reviewed changes

lukedirtwalker force-pushed the pubGoIntegrationTesting branch from 1f7ca55 to 5ef9b66 Compare July 27, 2018 08:21

lukedirtwalker commented Jul 27, 2018

View reviewed changes

shitz approved these changes Jul 27, 2018

View reviewed changes

worxli approved these changes Jul 30, 2018

View reviewed changes

kormat suggested changes Jul 30, 2018

View reviewed changes

kormat reviewed Jul 30, 2018

View reviewed changes

lukedirtwalker commented Jul 30, 2018

View reviewed changes

lukedirtwalker force-pushed the pubGoIntegrationTesting branch from b2c851b to 254abfc Compare July 30, 2018 13:39

kormat suggested changes Jul 30, 2018

View reviewed changes

lukedirtwalker commented Jul 31, 2018

View reviewed changes

kormat approved these changes Jul 31, 2018

View reviewed changes

Initial go integration test framework

3db0a15

Fixes scionproto#1693

lukedirtwalker force-pushed the pubGoIntegrationTesting branch from 27ed805 to 3db0a15 Compare July 31, 2018 10:35

lukedirtwalker merged commit 74b9299 into scionproto:master Jul 31, 2018

lukedirtwalker deleted the pubGoIntegrationTesting branch July 31, 2018 10:48

lukedirtwalker added a commit that referenced this pull request Dec 16, 2019

snet: refactor Listens and Dials (#1694) (#3521)

c2b6c9f

- Remove bind - Use a single method Listen / Dial - Use {s}net.UDPAddr as arguments - Improve naming of arguments Contributes #3136

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial go integration test framework #1694

Initial go integration test framework #1694

lukedirtwalker commented Jul 19, 2018 •

edited by kormat

Loading

worxli left a comment

lukedirtwalker left a comment

worxli left a comment

worxli left a comment

lukedirtwalker left a comment

shitz left a comment

lukedirtwalker left a comment

shitz left a comment

lukedirtwalker left a comment

shitz left a comment

kormat left a comment

worxli left a comment

lukedirtwalker left a comment

shitz left a comment

kormat left a comment

worxli left a comment

lukedirtwalker left a comment

lukedirtwalker left a comment

shitz left a comment

kormat left a comment

kormat left a comment

lukedirtwalker left a comment

kormat left a comment

lukedirtwalker left a comment

kormat left a comment

Initial go integration test framework #1694

Initial go integration test framework #1694

Conversation

lukedirtwalker commented Jul 19, 2018 • edited by kormat Loading

worxli left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

worxli left a comment

Choose a reason for hiding this comment

worxli left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

shitz left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

shitz left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

shitz left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

worxli left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

shitz left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

worxli left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

shitz left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

lukedirtwalker left a comment

Choose a reason for hiding this comment

kormat left a comment

Choose a reason for hiding this comment

lukedirtwalker commented Jul 19, 2018 •

edited by kormat

Loading