deployer: add a bunch of test coverage and fix a few panics #20694

rboyer · 2024-02-21T20:21:17Z

Description

This adds a bunch of coverage of the topology.Compile method. It is not complete, but it is a start.

A few panics and miscellany were fixed.
The testing/deployer tests are now also run in CI.

rboyer · 2024-02-21T20:21:40Z

testing/deployer/topology/compile.go

 }

-func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error) {
+func compile(logger hclog.Logger, raw *Config, prev *Topology, testingID string) (*Topology, error) {


Adding this so that we can control the id assignment during testing.

rboyer · 2024-02-21T20:22:15Z

testing/deployer/topology/compile.go

@@ -90,22 +100,17 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
 		nextIndex int // use a global index so any shared networks work properly with assignments
 	)

-	foundPeerNames := make(map[string]map[string]struct{})
-	for _, c := range raw.Clusters {
+	compileCluster := func(c *Cluster) (map[string]struct{}, error) {


A follow-on PR can extract these anonymous methods as real methods. I did it this way to make it more obvious where the bugfixes were, and to add error logging context.

rboyer · 2024-02-21T20:23:10Z

testing/deployer/topology/compile.go

@@ -158,6 +162,7 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
 				m = make(map[string]struct{})
 				tenancies[partition] = m
 			}
+			m["default"] = struct{}{}


One of the ways to introduce a tenancy would inject the default, but this one did not. Correcting this omission. The system will automatically create this anyway so this just accurately reflects the tenancies that will exist.

rboyer · 2024-02-21T20:23:43Z

testing/deployer/topology/compile.go

@@ -238,10 +242,9 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
 			}
 			addTenancy(n.Partition, "default")

-			if _, exists := seenNodes[n.ID()]; exists {
-				return nil, fmt.Errorf("cannot have two nodes in the same cluster %q with the same name %q", c.Name, n.ID())
+			if n.IsServer() && n.Partition != "default" {


Adding this restriction. Practically this would have failed in consul itself, but this makes it fail up higher where it's more actionable.

rboyer · 2024-02-21T20:24:06Z

testing/deployer/topology/compile.go

@@ -476,6 +482,15 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
 								Protocol:   cfg.ActualProtocol,
 							})
 						}
+						sort.Slice(svcPorts, func(i, j int) bool {


This makes the output deterministic to aid in unit test comparisons.

rboyer · 2024-02-21T20:24:50Z

testing/deployer/topology/compile.go

+			maps.Copy(foundPeerNames, peerNames)
+		}
+
+		// Default anything in the toplevel services map.


Omission: many of the pathways to implicitly create this v2 services map will default the protocol, but the BYO map does not.

rboyer · 2024-02-21T20:25:12Z

testing/deployer/topology/topology.go

@@ -898,6 +902,9 @@ func (w *Workload) ports() []int {
 	if len(w.Ports) > 0 {
 		seen := make(map[int]struct{})
 		for _, port := range w.Ports {
+			if port == nil {


Panic if no ports were defined.

rboyer · 2024-02-21T20:25:22Z

testing/deployer/topology/topology.go

 					Number:   w.Port,
 					Protocol: "tcp",
 				},
 			}
 			w.Port = 0
 		}
+		if w.Ports == nil {


Another panic.

rboyer · 2024-02-21T20:25:27Z

testing/deployer/topology/topology.go

@@ -990,7 +1002,7 @@ func (w *Workload) Validate() error {
 		}
 	} else {
 		if len(w.Ports) > 0 {
-			return fmt.Errorf("cannot specify mulitport on service in v1")
+			return fmt.Errorf("cannot specify multiport on service in v1")


rboyer · 2024-02-21T20:26:07Z

testing/deployer/topology/compile_test.go

+	Workload{},
+}
+
+func assertDeepEqual[V any](t testingT, exp, got V, msgAndArgs ...any) {


This is an inlined, modified version of prototest.AssertDeepEqual for use in this module.

rboyer · 2024-02-21T20:26:50Z

testing/deployer/topology/compile_test.go

+		}
+	}
+
+	cases := map[string]testcase{


The cases covered here test some happy paths, and most of the unhappy paths.

NiniOak · 2024-02-21T22:05:04Z

testing/deployer/topology/compile.go

 	"github.com/hashicorp/consul/testing/deployer/util"
 )

 const DockerPrefix = "cslc" // ConSuLCluster

 func Compile(logger hclog.Logger, raw *Config) (*Topology, error) {
-	return compile(logger, raw, nil)
+	return compile(logger, raw, nil, "")
 }

 func Recompile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error) {


Why is testingID not added to the parameters required for Recompile function?

I would not want to expose the testingID parameter in an exported function yet, as it is just there for the package tests to use.

Possibly if deployer-meta-tests get written for stuff outside of the topology package we might revisit this. It seemed easiest to keep this testing detail internal to this package to start.

NiniOak · 2024-02-21T22:06:46Z

testing/deployer/topology/compile.go

 	"github.com/hashicorp/consul/testing/deployer/util"
 )

 const DockerPrefix = "cslc" // ConSuLCluster

 func Compile(logger hclog.Logger, raw *Config) (*Topology, error) {
-	return compile(logger, raw, nil)
+	return compile(logger, raw, nil, "")


Shouldn't the testingID value be set at a high level? Like wherever compile is called?

The tests in compile_test.go call the function this way:

got, err := compile(logger, tc.in, nil, clusterID)

I'm not calling Compile, but rather the unexported compile and using the testingID field which is otherwise inaccessible.

NiniOak

Few questions but non blockers, thanks for these updates

deployer: add a bunch of test coverage and fix a few panics

94c5c75

rboyer commented Feb 21, 2024

View reviewed changes

rboyer added 3 commits February 21, 2024 14:29

mark the integration tests as short

60a6c1e

switch to a build tag to skip integ tests

f13c915

test deployer stuff

b928ddf

rboyer added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test pr/no-backport labels Feb 21, 2024

and activate

68474d1

rboyer requested review from huikang and NiniOak February 21, 2024 20:42

rboyer self-assigned this Feb 21, 2024

rboyer added the theme/testing Testing, and related enhancements label Feb 21, 2024

rboyer marked this pull request as ready for review February 21, 2024 20:42

NiniOak reviewed Feb 21, 2024

View reviewed changes

NiniOak approved these changes Feb 21, 2024

View reviewed changes

rboyer merged commit 235747d into main Feb 22, 2024
91 checks passed

rboyer deleted the rboyer/deployer-unit-tests branch February 22, 2024 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deployer: add a bunch of test coverage and fix a few panics #20694

deployer: add a bunch of test coverage and fix a few panics #20694

rboyer commented Feb 21, 2024 •

edited

Loading

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

rboyer Feb 21, 2024

NiniOak Feb 21, 2024 •

edited

Loading

rboyer Feb 21, 2024

NiniOak Feb 21, 2024 •

edited

Loading

rboyer Feb 21, 2024

NiniOak left a comment

deployer: add a bunch of test coverage and fix a few panics #20694

deployer: add a bunch of test coverage and fix a few panics #20694

Conversation

rboyer commented Feb 21, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiniOak Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiniOak Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiniOak left a comment

Choose a reason for hiding this comment

rboyer commented Feb 21, 2024 •

edited

Loading

NiniOak Feb 21, 2024 •

edited

Loading

NiniOak Feb 21, 2024 •

edited

Loading