Following testing convention in TestOrchestration #167
Conversation
We skip the / separator for the first field since this is added by the t.Run(name) call.
meling
left a comment
meling
left a comment
There was a problem hiding this comment.
Looks very nice!! A few minor nits and one open question...
| return nil, err | ||
| } | ||
| opt.SetTreeOptions(e.hostCfg.BranchFactor, e.hostCfg.TreePositions, e.hostCfg.TreeDelta) | ||
| // opt.SetTreeOptions(e.hostCfg.BranchFactor, e.hostCfg.TreePositions, e.hostCfg.TreeDelta) |
There was a problem hiding this comment.
Keep! I forgot to uncomment this. On that note, we still might want to remove this line in a different branch since the tree values don't need to be set here when they can come from replicaOpts in NewExperiment instead
There was a problem hiding this comment.
Opened #168 to address this problem and some other stuff
| t.Run(test.Name([]string{"consensus", "crypto", "byzantine", "mods"}, tt.consensus, tt.crypto, tt.byzantine, tt.mods), func(t *testing.T) { | ||
| var replicaOpts *orchestrationpb.ReplicaOpts | ||
| if slices.Contains(tt.mods, "kauri") { | ||
| replicaOpts = makeTreeReplicaOpts(tt.consensus, tt.crypto, tt.mods, 7, 2, tt.randomTree) |
There was a problem hiding this comment.
We should make 7, 2 be part of the tests struct, and leave it unspecified (0) for the non-kauri tests.
| mods []string | ||
| randomTree bool | ||
| }{ | ||
| {consensus: "chainedhotstuff", crypto: "ecdsa", byzantine: "", mods: nil}, |
There was a problem hiding this comment.
we should remove mods: nil from lines that don't need it.
There was a problem hiding this comment.
the same applies to byzantine I think; it will be the zero value anyway.
| workerProxy := orchestration.NewRemoteWorker(protostream.NewWriter(controllerStream), protostream.NewReader(controllerStream)) | ||
| worker := orchestration.NewWorker(protostream.NewWriter(workerStream), protostream.NewReader(workerStream), metrics.NopLogger(), nil, 0) | ||
|
|
||
| cfg := config.NewLocal(7, 2) |
There was a problem hiding this comment.
should these be moved into the tests struct?
…tuff into 159-testorchestration-cleanup
Fixes #159