test: use `T.Setenv` to set env vars in tests #4126

Juneezee · 2022-08-17T15:18:26Z

The Problem/Issue/Bug:

How this PR Solves The Problem:

This PR replaces os.Setenv with t.Setenv. Starting from Go 1.17, we can use t.Setenv to set environment variable in test. The environment variable is automatically restored to its original value when the test and all its subtests complete. This ensures that each test does not start with leftover environment variables from previous completed tests.

This saves us at least 2 lines (error check, and unsetting the env var) on every instance.

Reference: https://pkg.go.dev/testing#T.Setenv

Manual Testing Instructions:

Automated Testing Overview:

Release/Deployment notes:

github-actions · 2022-08-17T18:48:31Z

Download the artifacts for this pull request:

rfay

Awesome, thanks! I didn't know that. It's a great improvement. Your improvements to tests (and anything else) are always welcome.

rfay

Looks like this has broken TestShareCmd, but I'm baffled how. Probably something that runs before TestShareCmd isn't cleaning up like it was? I'm running all the tests in cmd to see if I can recreate it locally. I'm unable to recreate just running TestShareCmd.

Juneezee · 2022-08-19T10:48:25Z

@rfay Looks like it is TestPoweroffOnNewVersion that fails. Although I'm not sure why the logs are grouped incorrectly.

rfay · 2022-08-19T12:28:48Z

Pretty sure it's TestShareCmd. Never have understood why the grouping is wrong, Have explored it a little.

That said... it appears to be something else that's breaking it. I'll try running it right on the test runner though. I've only tried recreating elsewhere so far.

rfay · 2022-08-23T02:09:17Z

Finally got TestShareCmd to fail locally with targets
(TestGetActiveAppRoot|TestPoweroffOnNewVersion|TestCmdService|TestShareCmd)
and environment
DDEV_BINARY_FULLPATH=/Users/rfay/workspace/ddev/.gotmp/bin/darwin_arm64/ddev;DDEV_DEBUG=true;DDEV_TEST_SHARE_CMD=true;GOTEST_SHORT=8

rfay · 2022-08-23T02:19:19Z

$ export DDEV_BINARY_FULLPATH=/Users/rfay/workspace/ddev/.gotmp/bin/darwin_arm64/ddev DDEV_DEBUG=true DDEV_TEST_SHARE_CMD=true GOTEST_SHORT=8
rfay@rfay-mbp-2021:~/workspace/ddev$ make testcmd TESTARGS="-run '(TestGetActiveAppRoot|TestPoweroffOnNewVersion|TestCmdService|TestShareCmd)'"
building .gotmp/bin/darwin_arm64/ddev from ./cmd/... ./pkg/...
LDFLAGS=-extldflags -static -X github.com/drud/ddev/pkg/versionconstants.DdevVersion=v1.21.1-1-gbba8b50dc -X github.com/drud/ddev/pkg/versionconstants.SegmentKey=CawBO33fRNynkaZsfgjY8sTxDT3yrH9c
DDEV_BINARY_FULLPATH=/Users/rfay/workspace/ddev/.gotmp/bin/darwin_arm64/ddev
export PATH="/Users/rfay/workspace/ddev/.gotmp/bin/darwin_arm64:$PATH" DDEV_NO_INSTRUMENTATION=true CGO_ENABLED=0 DDEV_BINARY_FULLPATH=/Users/rfay/workspace/ddev/.gotmp/bin/darwin_arm64/ddev; go test  -p 1 -timeout 4h -v -installsuffix static -ldflags " -extldflags -static -X 'github.com/drud/ddev/pkg/versionconstants.DdevVersion=v1.21.1-1-gbba8b50dc' -X 'github.com/drud/ddev/pkg/versionconstants.SegmentKey=CawBO33fRNynkaZsfgjY8sTxDT3yrH9c' " ./cmd/... -run '(TestGetActiveAppRoot|TestPoweroffOnNewVersion|TestCmdService|TestShareCmd)'
?   	github.com/drud/ddev/cmd/ddev	[no test files]
time="2022-08-22T20:15:44-06:00" level=info msg="Running ddev with ddev= /Users/rfay/workspace/ddev/.gotmp/bin/darwin_arm64/ddev"
time="2022-08-22T20:15:44-06:00" level=debug msg="Preparing TestSites"
time="2022-08-22T20:15:44-06:00" level=debug msg="Preparing TestCmdWordpress"
Copying directory /Users/rfay/.ddev/testcache/TestCmdWordpress/TestCmdWordpress_siteArchive to /Users/rfay/tmp/ddevtest/TestCmdWordpress3720424298

Copying complete
time="2022-08-22T20:15:45-06:00" level=debug msg="Config Directory does not exist, attempting to create." directory=/Users/rfay/tmp/ddevtest/TestCmdWordpress3720424298/.ddev
time="2022-08-22T20:15:45-06:00" level=debug msg="Adding TestSites"
time="2022-08-22T20:15:45-06:00" level=debug msg="Removing any existing TestSites"
time="2022-08-22T20:15:46-06:00" level=debug msg="Starting TestSites"
time="2022-08-22T20:16:02-06:00" level=debug msg="Running tests."
=== RUN   TestGetActiveAppRoot
Creating database snapshot TestCmdWordpress_remove_data_snapshot_20220822201602
Flushed mutagen sync session 'TestCmdWordpress'
Terminated mutagen sync session 'TestCmdWordpress'
Container ddev-TestCmdWordpress-db  Removed
Container ddev-TestCmdWordpress-dba  Removed
Container ddev-TestCmdWordpress-web  Removed
Network ddev-testcmdwordpress_default  Removed
Pulling image for drud/ddev-webserver:20220808_yarn_crash
Pulling image for busybox:stable
Pulling image for drud/ddev-ssh-agent:v1.20.0
Pulling image for drud/ddev-router:v1.20.0
Copied /Users/rfay/Library/Application Support/mkcert:CopyIntoVolume_qdmdxrqmthoh into /mnt/v/mkcert in 110.323291ms
Exec chown -R 501 /mnt/v/mkcert stdout=, stderr=, err=<nil>
Pushed mkcert rootca certs to ddev-global-cache/mkcert
Pulling image for drud/ddev-dbserver-mariadb-10.4:v1.20.0
Pulling image for phpmyadmin:5
Pulling image for drud/ddev-webserver:20220808_yarn_crash
Pulling image for drud/ddev-router:v1.20.0
Pulling image for drud/ddev-ssh-agent:v1.20.0
IsInternetActive DEBUG: err=<nil> ctx.Err()=<nil> addrs=[127.0.0.1] IsInternetactive==true, randomURL=agybotkqxp.ddev.site internet_detection_timeout_ms=3000ms

Executing docker-compose -f /Users/rfay/tmp/ddevtest/TestCmdWordpress3720424298/.ddev/.ddev-docker-compose-full.yaml up --build -d
Network ddev-testcmdwordpress_default  Created
Container ddev-TestCmdWordpress-web  Started
Container ddev-TestCmdWordpress-dba  Started
Container ddev-TestCmdWordpress-db  Started
Starting mutagen sync process... This can take some time.
chowning mutagen docker volume for user 501
done chowning mutagen docker volume; result=<nil>
Using mutagen config file /Users/rfay/tmp/ddevtest/TestCmdWordpress3720424298/.ddev/mutagen/mutagen.yml
Terminating mutagen sync if session already exists
Creating mutagen sync: mutagen [sync create /Users/rfay/tmp/ddevtest/TestCmdWordpress3720424298 docker://8e7f64de0e0016dc6348281010bcb6fc50bafedad9f861fbb86a69dde42d453c/var/www/html --no-global-configuration --name TestCmdWordpress --configuration-file=/Users/rfay/tmp/ddevtest/TestCmdWordpress3720424298/.ddev/mutagen/mutagen.yml]
Flushing mutagen sync session 'TestCmdWordpress'
Flushed mutagen sync session 'TestCmdWordpress'
Mutagen sync flush completed in 2s.
For details on sync status 'ddev mutagen status TestCmdWordpress --verbose'
Container ddev-router  Started
--- PASS: TestGetActiveAppRoot (19.64s)
=== RUN   TestPoweroffOnNewVersion
Command  Result=IsInternetActive DEBUG: err=<nil> ctx.Err()=<nil> addrs=[127.0.0.1] IsInternetactive==true, randomURL=ajesmhsdmx.ddev.site internet_detection_timeout_ms=3000ms

Network ddev_default created
Failed to get project(s): could not find requested project 'TestCmdWordpress', you may need to use "ddev start" to add it to the project catalog

--- PASS: TestPoweroffOnNewVersion (51.80s)
=== RUN   TestCmdService
--- PASS: TestCmdService (1.62s)
=== RUN   TestShareCmd
Scanning all done at 2022-08-22 20:17:16.286355 -0600 MDT m=+91.891765459
    share_test.go:122: no URL found: map[]
--- FAIL: TestShareCmd (0.76s)
FAIL
Command  Result=Project TestCmdWordpress is already stopped.
mutagen sync session 'TestCmdWordpress' is not working correctly: --------------------------------------------------------------------------------
Name: TestCmdWordpress
Identifier: sync_O5Df9CntVt0bxHABjwahW5YZjSXZfpJYxg2UkWEIbT1
Alpha:
	URL: /Users/rfay/tmp/ddevtest/TestCmdWordpress3720424298
	Connected: Yes
Beta:
	URL: docker://8e7f64de0e0016dc6348281010bcb6fc50bafedad9f861fbb86a69dde42d453c/var/www/html
		DOCKER_HOST=unix:///Users/rfay/.colima/default/docker.sock
	Connected: No
Last error: beta polling error: unable to receive poll response: unable to read message length: unexpected EOF
Status: Connecting to beta
--------------------------------------------------------------------------------

Terminated mutagen sync session 'TestCmdWordpress'
Volume TestCmdWordpress-mariadb for project TestCmdWordpress was deleted
Volume TestCmdWordpress-postgres for project TestCmdWordpress was deleted
Volume TestCmdWordpress_project_mutagen for project TestCmdWordpress was deleted
Deleted docker image drud/ddev-dbserver-mariadb-10.4:v1.20.0-TestCmdWordpress-built
Deleted docker image drud/ddev-webserver:20220808_yarn_crash-TestCmdWordpress-built
Project TestCmdWordpress was deleted. Your code and configuration are unchanged.
Project TestCmdWordpress has been stopped.
IsInternetActive DEBUG: err=<nil> ctx.Err()=<nil> addrs=[127.0.0.1] IsInternetactive==true, randomURL=qgbpcigqvf.ddev.site internet_detection_timeout_ms=3000ms


FAIL	github.com/drud/ddev/cmd/ddev/cmd	93.080s
?   	github.com/drud/ddev/cmd/ddev_gen_autocomplete	[no test files]
FAIL
make: *** [testcmd] Error 1

rfay · 2022-08-23T02:27:22Z

It's TestPoweroffOnNewVersion() that breaks it, and that had changes made.

rfay · 2022-08-23T02:34:30Z

Removing your two changes to TestPoweroffOnNewVersion() resolves the issue, but I'll leave it to you to figure out why! I'd sure like to know. Apparently it's not ending up with the correct env vars. I guess just adding debugs to TestShareCmd with those env vars will explain it.

This commit replaces `os.Setenv` with `t.Setenv` in tests. The environment variable is automatically restored to its original value when the test and all its subtests complete. Reference: https://pkg.go.dev/testing#T.Setenv Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>

Juneezee · 2022-08-23T04:45:45Z

@rfay Thanks for the helpful debugging ❤️ ! The issue was the cleanup order. Since t.Cleanup will be called in last added, first called order (go doc), cleanups that rely on original HOME or USERPROFILE will fail.

I have changed the cleanup order of TestPoweroffOnNewVersion in cab0851, please take a look. Thanks 😃

Prior to the commit, the cleanup from `t.Setenv` will be called last. This would cause other cleanups that rely on original HOME / USERPROFILE environment variables to fail. Reference: #4126 (comment) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>

Juneezee · 2022-08-23T07:30:50Z

cmd/ddev/cmd/root_test.go

+	t.Cleanup(func() {
+		err = os.Chdir(tmpJunkProject)
+		assert.NoError(err)
+		_, _ = exec.RunHostCommand(DdevBin, "delete", "-Oy")
+
+		err = os.Chdir(origDir)
+		assert.NoError(err)
+		_ = os.RemoveAll(tmpJunkProject)
+
+		// Because the start has done a poweroff (new ddev version),
+		// make sure sites are running again.
+		for _, site := range TestSites {
+			_, _ = exec.RunCommand(DdevBin, []string{"start", "-y", site.Name})
+		}
+	})


This cleanup needs the original HOME / USERPROFILE, so it must be called after the cleanup from t.Setenv below.

rfay · 2022-08-23T13:03:53Z

Awesome, congrats!

rfay

Thanks so much! I fiddled with TestPoweroffOnNewVersion just a bit because I wanted to manually test it properly, and didn't like some of the things I'd done, anyway, tried to improve it a bit. It not a great test, but does its job I guess.

rfay · 2022-08-23T22:35:03Z

Thanks so much for this! Great work, and great to know about t.Setenv()

rfay approved these changes Aug 17, 2022

View reviewed changes

rfay reviewed Aug 18, 2022

View reviewed changes

Juneezee commented Aug 23, 2022

View reviewed changes

Minor fiddling with TestDdevPoweroffOnNewVersion, naming issues

792407a

rfay approved these changes Aug 23, 2022

View reviewed changes

rfay merged commit 7b5430a into ddev:master Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: use `T.Setenv` to set env vars in tests #4126

test: use `T.Setenv` to set env vars in tests #4126

Juneezee commented Aug 17, 2022 •

edited by gitpod-io bot

github-actions bot commented Aug 17, 2022 •

edited

rfay left a comment

rfay left a comment

Juneezee commented Aug 19, 2022

rfay commented Aug 19, 2022

rfay commented Aug 23, 2022

rfay commented Aug 23, 2022

rfay commented Aug 23, 2022

rfay commented Aug 23, 2022

Juneezee commented Aug 23, 2022 •

edited

Juneezee Aug 23, 2022

rfay commented Aug 23, 2022

rfay left a comment

rfay commented Aug 23, 2022

test: use T.Setenv to set env vars in tests #4126

test: use T.Setenv to set env vars in tests #4126

Conversation

Juneezee commented Aug 17, 2022 • edited by gitpod-io bot

The Problem/Issue/Bug:

How this PR Solves The Problem:

Manual Testing Instructions:

Automated Testing Overview:

Related Issue Link(s):

Release/Deployment notes:

github-actions bot commented Aug 17, 2022 • edited

rfay left a comment

Choose a reason for hiding this comment

rfay left a comment

Choose a reason for hiding this comment

Juneezee commented Aug 19, 2022

rfay commented Aug 19, 2022

rfay commented Aug 23, 2022

rfay commented Aug 23, 2022

rfay commented Aug 23, 2022

rfay commented Aug 23, 2022

Juneezee commented Aug 23, 2022 • edited

Juneezee Aug 23, 2022

Choose a reason for hiding this comment

rfay commented Aug 23, 2022

rfay left a comment

Choose a reason for hiding this comment

rfay commented Aug 23, 2022

test: use `T.Setenv` to set env vars in tests #4126

test: use `T.Setenv` to set env vars in tests #4126

Juneezee commented Aug 17, 2022 •

edited by gitpod-io bot

github-actions bot commented Aug 17, 2022 •

edited

Juneezee commented Aug 23, 2022 •

edited