test: support running integration test with envoy bin #810

spacewander · 2024-11-28T02:07:53Z

No description provided.

codecov · 2024-11-28T02:13:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.72%. Comparing base (97a14ea) to head (6c22db2).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
+ Coverage   85.68%   85.72%   +0.03%     
==========================================
  Files         141      141              
  Lines        8336     8336              
==========================================
+ Hits         7143     7146       +3     
+ Misses        937      934       -3     
  Partials      256      256

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: spacewander <[email protected]>

zll600 · 2024-11-28T13:28:25Z

api/plugins/tests/integration/dataplane/bootstrap.go

+	portEnv, _ := strconv.Atoi(os.Getenv("TEST_ENVOY_CONTROL_PLANE_PORT"))
+	if portEnv != 0 {
 		port = portEnv
 	}


I prefer the following method, but yours is okay.

Suggested change

portEnv, _ := strconv.Atoi(os.Getenv("TEST_ENVOY_CONTROL_PLANE_PORT"))

if portEnv != 0 {

port = portEnv

}

if p, err := strconv.Atoi(os.Getenv("TEST_ENVOY_CONTROL_PLANE_PORT")); err == nil && p != 0 {

port = p

}

No, thanks! My code style is more concise.

zll600 · 2024-11-28T13:34:22Z

api/plugins/tests/integration/dataplane/bootstrap.go

+	addr["port_value"] = b.dp.Port()
+
+	if isBinaryMode() {
+		for _, l := range root["static_resources"].(map[string]interface{})["listeners"].([]interface{}) {


can we use staticResource defined above here?

Suggested change

for _, l := range root["static_resources"].(map[string]interface{})["listeners"].([]interface{}) {

for _, l := range staticResource["listeners"].([]interface{}) {

zll600 · 2024-11-28T13:38:00Z

api/plugins/tests/integration/dataplane/bootstrap.go

+			httpFilters := hcm["http_filters"].([]interface{})
+			for _, hf := range httpFilters {


It looks like httpFilters is only used once. I prefer to remove this middle variable. But reserving it is also good.

Suggested change

httpFilters := hcm["http_filters"].([]interface{})

for _, hf := range httpFilters {

for _, hf := range hcm["http_filters"].([]interface{}) {

zll600 · 2024-11-28T13:42:29Z

api/plugins/tests/integration/dataplane/data_plane.go

+
+	err = opt.Bootstrap.WriteTo(cfgFile)
+	cfgFile.Close()
+	if err != nil {
+		return nil, err
+	}


cfgFile is used below. We can not close it here immediately.

Suggested change

err = opt.Bootstrap.WriteTo(cfgFile)

cfgFile.Close()

if err != nil {

return nil, err

}

defer cfgFile.Close()

if err = opt.Bootstrap.WriteTo(cfgFile); err != nil {

return nil, err

}

We only use cfgFile.Name() later

In fact, we must close the cfgFile immediately to ensure the written content is flushed.

OK, Thank you for your detailed explanation.

zll600 · 2024-11-28T13:45:41Z

api/plugins/tests/integration/dataplane/data_plane.go

+		image := "m.daocloud.io/docker.io/envoyproxy/envoy:contrib-v1.32.0"
+
+		specifiedImage := os.Getenv("PROXY_IMAGE")
+		if specifiedImage != "" {
+			image = specifiedImage
+		}


Suggested change

image := "m.daocloud.io/docker.io/envoyproxy/envoy:contrib-v1.32.0"

specifiedImage := os.Getenv("PROXY_IMAGE")

if specifiedImage != "" {

image = specifiedImage

}

image := "m.daocloud.io/docker.io/envoyproxy/envoy:contrib-v1.32.0"

if specifiedImage := os.Getenv("PROXY_IMAGE"); specifiedImage != "" {

image = specifiedImage

}

zll600 · 2024-11-28T13:51:19Z

api/plugins/tests/integration/dataplane/data_plane.go

+			hostAddr + " " +
+			image
+
+		envoyCmd = "envoy -c /etc/envoy.yaml"


the construct of envoyCmd is simple, what about moving it before cmdLine?

This is too trivial to change.

The cmdLine doesn't depend on the envoyCmd. And envoyCmd is a new neighbor of the cmdLine. Add the new code after the old one is natural.

OK, it makes sense to me.

zll600 · 2024-11-28T13:54:35Z

api/plugins/tests/integration/dataplane/data_plane.go

+			addEnvironemntVariables(cmd, opt.Envs)
+		}
+
+		out, err := cmd.CombinedOutput()


it looks like we can use following pattern here:

if out, err := cmd.CombinedOutput(); err != nil { ... }

zll600 · 2024-11-28T13:58:00Z

api/plugins/tests/integration/dataplane/data_plane.go

+			return nil, err
+		}
+		cmd.Stdout = stdout
+


the logic for stdout are same in two branches here. So I think we can move the logic of stdout out of the if-else here.

It will looks like:

if stdout, err := os.Create(filepath.Join(dir, "stdout")); err != nil { cmd.Stdout = stdout } if isBinaryMode() { cmd.Stderr = .... }

I prefer to keep the code in the if-else block so I can write comments for it, especially to make the comment match the one in the stderr.

zll600 · 2024-11-28T13:59:03Z

api/plugins/tests/integration/dataplane/data_plane.go

+		waitTimeEnv, _ := time.ParseDuration(os.Getenv("TEST_ENVOY_WAIT_BINARY_TO_START_TIME"))
+		if waitTimeEnv != 0 {
+			waitTime = waitTimeEnv
+		}


Suggested change

waitTimeEnv, _ := time.ParseDuration(os.Getenv("TEST_ENVOY_WAIT_BINARY_TO_START_TIME"))

if waitTimeEnv != 0 {

waitTime = waitTimeEnv

}

if waitTimeEnv, err := time.ParseDuration(os.Getenv("TEST_ENVOY_WAIT_BINARY_TO_START_TIME")); err != nil && waitTimeEnv != 0 {

waitTime = waitTimeEnv

}

zll600 · 2024-11-28T14:00:24Z

api/plugins/tests/integration/dataplane/data_plane.go

+		dp.cmd.Process.Signal(syscall.SIGTERM)
+	} else {
+		cmd := exec.Command("docker", "stop", containerName)
+		err = cmd.Run()


I think that we can check err here immediately, another branch will not return an error.

This is too trivial to change. If we add the err next time, we have to move the err check out again.

zll600 · 2024-11-28T14:05:46Z

If my points have mistakes, feel free to correct me.

github-actions bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2024

spacewander force-pushed the aci_run_intt branch 4 times, most recently from 228bae8 to b5be4c0 Compare November 28, 2024 03:21

github-actions bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 28, 2024

spacewander force-pushed the aci_run_intt branch 3 times, most recently from 0fc3dfd to 5d34462 Compare November 28, 2024 08:15

github-actions bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 28, 2024

test: support running integration test with envoy bin

6c22db2

Signed-off-by: spacewander <[email protected]>

spacewander force-pushed the aci_run_intt branch from 5d34462 to 6c22db2 Compare November 28, 2024 08:20

spacewander marked this pull request as ready for review November 28, 2024 08:31

zll600 reviewed Nov 28, 2024

View reviewed changes

spacewander merged commit 855823f into main Nov 29, 2024
20 checks passed

spacewander deleted the aci_run_intt branch November 29, 2024 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: support running integration test with envoy bin #810

test: support running integration test with envoy bin #810

spacewander commented Nov 28, 2024

codecov bot commented Nov 28, 2024 •

edited

Loading

zll600 Nov 28, 2024

spacewander Nov 29, 2024

zll600 Nov 28, 2024

zll600 Nov 28, 2024

zll600 Nov 28, 2024

spacewander Nov 29, 2024

spacewander Nov 29, 2024

zll600 Nov 29, 2024

zll600 Nov 28, 2024

zll600 Nov 28, 2024

spacewander Nov 29, 2024

spacewander Nov 29, 2024

zll600 Nov 29, 2024

zll600 Nov 28, 2024

zll600 Nov 28, 2024

spacewander Nov 29, 2024

zll600 Nov 28, 2024

zll600 Nov 28, 2024

spacewander Nov 29, 2024

zll600 commented Nov 28, 2024

	for _, l := range root["static_resources"].(map[string]interface{})["listeners"].([]interface{}) {
	for _, l := range staticResource["listeners"].([]interface{}) {

		httpFilters := hcm["http_filters"].([]interface{})
		for _, hf := range httpFilters {

	httpFilters := hcm["http_filters"].([]interface{})
	for _, hf := range httpFilters {
	for _, hf := range hcm["http_filters"].([]interface{}) {

test: support running integration test with envoy bin #810

test: support running integration test with envoy bin #810

Conversation

spacewander commented Nov 28, 2024

codecov bot commented Nov 28, 2024 • edited Loading

Codecov Report

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

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

zll600 commented Nov 28, 2024

codecov bot commented Nov 28, 2024 •

edited

Loading