Reviewed 30 of 30 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @lukedirtwalker)

acceptance/topo_ps_reload/reload_test.go, line 27 at r1 (raw file):

	"time"

	"github.com/scionproto/scion/go/lib/topology"

here there should be a lint error

acceptance/topo_ps_reload/reload_test.go, line 57 at r1 (raw file):

		"-d", "topo_ps_reload_dispatcher", "topo_ps_reload_path_srv")
	// wait a bit to make sure the containers are ready.
	time.Sleep(time.Second)

someone in the team does not like sleeps ;)
Ideally we want to ask docker-compose ps and see all are up and running, if not test should not proceed.

acceptance/topo_ps_reload/reload_test.go, line 82 at r1 (raw file):

}

func mustExec(t *testing.T, name string, arg ...string) {

maybe you need t.helper()
personally I like the help function not to do assert but to return the out,err

#in the main test 
out,err:=mustExec( ...)
assert.NoError(t, err, fmt.SprintF("failed with output %s",out))

acceptance/topo_ps_reload/reload_test.go, line 89 at r1 (raw file):

}

func checkTopology(t *testing.T, expectedTopo *topology.RWTopology) {

do we need t.Helper()?

acceptance/topo_ps_reload/reload_test.go, line 92 at r1 (raw file):

	eJSON, err := json.Marshal(expectedTopo)
	require.NoError(t, err)
	actualTopo := fetchTopologyFromEndpoint(t, "http://242.253.100.2:30453/topology")

since we are in testing we could also exec to curl

acceptance/topo_ps_reload/reload_test.go, line 98 at r1 (raw file):

}

func fetchTopologyFromEndpoint(t *testing.T, url string) *topology.RWTopology {

ditto, also in theory we need ctx (timeout)

acceptance/topo_sd_reload/BUILD.bazel, line 7 at r1 (raw file):

    name = "topo_sd_reload_test",
    srcs = [
        "reload_test.go",

do you manually construct this files, or there is bazel command?

acceptance/topo_sd_reload/reload_test.go, line 36 at r1 (raw file):

	setupTest(t)
	defer teardownTest(t)
	// use a subtest to make sure that teardown is always executed.

what does it mean?

acceptance/topo_sd_reload/reload_test.go, line 52 at r1 (raw file):

	// first load the docker images from bazel into the docker deamon, the
	// scripts are in the same folder as this test runs in bazel.
	mustExec(t, "dispatcher")

? that is running locally the command ./dispatcher?

acceptance/topo_sd_reload/reload_test.go, line 68 at r1 (raw file):

	require.NoError(t, os.MkdirAll(fmt.Sprintf("%s/logs", outdir), os.ModePerm|os.ModeDir))
	// collect logs
	for _, file := range []string{"disp_1-ff00_0_110.log", "sd1-ff00_0_110.log"} {

Idea, when you have docker run you can specify the volume -v /local/path/:/usr/share/

maybe we could leverage that in docker-compose to just pack everything in an easy way

Also another idea is to use https://docs.docker.com/compose/reference/logs/ rather than working with files.

(suggestion and discussion, not really block on that)

acceptance/topo_sd_reload/testdata/disp.toml, line 5 at r1 (raw file):

[features]
AllowRunAsRoot = true

do you know how is this required? Is it always true?

go/lib/common/defs.go, line 53 at r1 (raw file):

}

func (ifid *IFIDType) UnmarshalJSON(text []byte) error {

Maybe a dummy comment

go/lib/common/defs_test.go, line 44 at r1 (raw file):

}

func TestJSONUnmarshalIFIDType(t *testing.T) {

is there a function JSONUnmarshalIFIDType?

go/lib/topology/overlay/defs.go, line 82 at r1 (raw file):

		return err
	}
	*ot = t

can we ot nil?

Reviewable status: 22 of 35 files reviewed, 14 unresolved discussions (waiting on @karampok)

acceptance/topo_ps_reload/reload_test.go, line 27 at r1 (raw file):

Done.

acceptance/topo_ps_reload/reload_test.go, line 57 at r1 (raw file):

:D yeah I heard so as well.
I don't think ps would be enough since the process internally also needs some time until it has the topology endpoint ready.
Here it's a fair tradeoff, the whole test takes 15s so 0.5s +/- isn't really that heavy.

acceptance/topo_ps_reload/reload_test.go, line 82 at r1 (raw file):

Done.

acceptance/topo_ps_reload/reload_test.go, line 89 at r1 (raw file):

Done.

acceptance/topo_ps_reload/reload_test.go, line 92 at r1 (raw file):

What would be the advantage of that? I think it's fine the way it is.

acceptance/topo_ps_reload/reload_test.go, line 98 at r1 (raw file):

Done. ctx would be nice, but since it's a test not strictly needed. If we see a lot of timeout we could still add it.

acceptance/topo_sd_reload/BUILD.bazel, line 7 at r1 (raw file):

Yeah those are manually written, but I now made sure that make gazelle also formats them.

acceptance/topo_sd_reload/reload_test.go, line 36 at r1 (raw file):

leftover, deleted it.

acceptance/topo_sd_reload/reload_test.go, line 52 at r1 (raw file):

yes, which is a bash script that loads the bazel docker image into the docker deamon. It's described in the comment above, let me know if we can improve documentation somehow.

acceptance/topo_sd_reload/reload_test.go, line 68 at r1 (raw file):

The problem with the volume approach is that the file is then owned by root, and on CI this leads to chaos ;)

docker-compose logs only outputs what is in stdout of the process. Well we could actually change the config to only print to stdout.

acceptance/topo_sd_reload/testdata/disp.toml, line 5 at r1 (raw file):

It is required because in this simple test we do not do any fancy su-exec in the docker container, thus we run as root, and the dispatcher binary checks that it can't run as root normally.

go/lib/common/defs.go, line 53 at r1 (raw file):

Done.

go/lib/common/defs_test.go, line 44 at r1 (raw file):

Done.

go/lib/topology/overlay/defs.go, line 82 at r1 (raw file):

It could be if you do struct{ t *overlay.Type } but I don't think that ever makes sense. Usually you would always use the value of overlay.Type and never the pointer.

Reviewed 13 of 14 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)

acceptance/topo_ps_reload/reload_test.go, line 57 at r1 (raw file):

the think is that sometime the setup test fails, and it would be nice to print that information
before execute the actual test.

I have been bitten by

1. looking what I might have broken in the functionality of the test
2. while the true error wast that the BS was crashing.

(not blocking discussing)

acceptance/topo_ps_reload/reload_test.go, line 98 at r1 (raw file):

with curl you have timeout for free and you can re-do the step manual

acceptance/topo_ps_reload/reload_test.go, line 99 at r2 (raw file):

}

func loadTopo(t *testing.T, name string) {

I don't know if also you need t.Helper() since that is two time nested.

(I personally return the errors, and asserts take place in main, so never checked how t.Helpe() is working when doubly/tripple nested)

acceptance/topo_sd_reload/reload_test.go, line 52 at r1 (raw file):

i would rename the script to something run-dispatcher-in-docker and remove the comment.
I cannot really find the script in the folder, is it something being built by basel?

acceptance/topo_sd_reload/reload_test.go, line 68 at r1 (raw file):

1. owned by root is a well known issue with (hopefully) a well known solution in containers
2. yes I am suggesting switch completely to only print to stdout

(no blocking request)

go/lib/common/defs_test.go, line 65 at r2 (raw file):

}

func TestIFIDTypeUnmarshalText(t *testing.T) {

t.Helper()

Reviewable status: 27 of 35 files reviewed, 3 unresolved discussions (waiting on @karampok)

acceptance/topo_ps_reload/reload_test.go, line 57 at r1 (raw file):

Added a "setup done" Log

acceptance/topo_ps_reload/reload_test.go, line 98 at r1 (raw file):

Running this test manual is difficult anyway. Like this it's more portable, you don't require curlto be installed.

acceptance/topo_ps_reload/reload_test.go, line 99 at r2 (raw file):

Done.

acceptance/topo_sd_reload/reload_test.go, line 52 at r1 (raw file):

it's built by bazel. The name is the same as the rule in the BUILD file.

acceptance/topo_sd_reload/reload_test.go, line 68 at r1 (raw file):

1. Not that I know of.
2. Done.

go/lib/common/defs_test.go, line 65 at r2 (raw file):

No that's an actual test?

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

acceptance/topo_ps_reload/reload_test.go, line 57 at r1 (raw file):

to my understanding, if the BS is crashing, the message will still be printed

acceptance/topo_sd_reload/reload_test.go, line 93 at r3 (raw file):

			fmt.Fprintf(os.Stderr, "Failed to read log for service %s: %v\n", service, err)
		}
		fmt.Printf("Writing file: %s", fmt.Sprintf("%s/logs/%s\n", outdir, file))

this should not be t.Logf?

Also (just informing of an alternative) you could have directly the stdout of a command to file

outfile, err := os.Create("./out.txt")
    if err != nil {
        panic(err)
    }
    defer outfile.Close()
    cmd.Stdout = outfile
``

acceptance/topo_sd_reload/testdata/disp.toml, line 8 at r3 (raw file):

[logging.console]
Level = "trace"

what is the default behavior in logging?
If we have no entries in toml where the logs go?
Can we that defined as args when running the binary (in docker compose file for example?)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karampok)

acceptance/topo_ps_reload/reload_test.go, line 57 at r1 (raw file):

add docker-compose ps

acceptance/topo_sd_reload/reload_test.go, line 93 at r3 (raw file):

Reviewed 3 of 3 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved