Skip to content

Add 2 modes test mechanism#9007

Merged
ulyssessouza merged 2 commits intodocker:v2from
ulyssessouza:add-test-modes
Dec 9, 2021
Merged

Add 2 modes test mechanism#9007
ulyssessouza merged 2 commits intodocker:v2from
ulyssessouza:add-test-modes

Conversation

@ulyssessouza
Copy link
Contributor

What I did
This should test on plugin and standalone mode

@ulyssessouza ulyssessouza force-pushed the add-test-modes branch 10 times, most recently from d747b1e to def6d16 Compare December 7, 2021 06:19
@ulyssessouza ulyssessouza marked this pull request as ready for review December 7, 2021 09:22
This should test on plugin and standalone mode

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>

func TestMain(m *testing.M) {
logrus.Info("Running tests on COMPOSE_STANDALONE_MODE=true")
_ = os.Setenv(composeStandaloneEnvVar, "true")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can’t we do this using a command line flag and a (global) variable instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My second option was to run the tests command line 2 times. One for each configuration of the envvar.
I'm not sure if I understood what you mean by using a global variable for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need an env var at all? Is this not the same process?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh... Finally got what you mean. The idea was to have it also configurable from outside of this TestMain.

With the envvar we can run just one specific test. And when running like this, TestMain doesn't run. So the envvar is a solution to be able to still pass this info to the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But can’t we use a command line flag instead?
Sorry it’s just I don’t like env vars much, they tend to magically configure (break) things, a bit like global vars on steroids 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only alternative I see is to pass a build tag when running the test to choose in between tagged files. So we can choose which file to use, so we can use that conditionally depending on the tag.... But that really looks like over engineering and gets to the same problematic in the end...

@ndeloof
Copy link
Contributor

ndeloof commented Dec 7, 2021

🏆

Comment on lines +205 to +210
const composeStandaloneEnvVar = "COMPOSE_STANDALONE_MODE"

// RunDockerComposeCmd runs a docker compose command, expects no error and returns a result
func (c *E2eCLI) RunDockerComposeCmd(args ...string) *icmd.Result {
composeStandaloneMode := os.Getenv(composeStandaloneEnvVar)
if composeStandaloneMode == "true" || composeStandaloneMode == "1" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const composeStandaloneEnvVar = "COMPOSE_STANDALONE_MODE"
// RunDockerComposeCmd runs a docker compose command, expects no error and returns a result
func (c *E2eCLI) RunDockerComposeCmd(args ...string) *icmd.Result {
composeStandaloneMode := os.Getenv(composeStandaloneEnvVar)
if composeStandaloneMode == "true" || composeStandaloneMode == "1" {
composeStandaloneMode := false
// RunDockerComposeCmd runs a docker compose command, expects no error and returns a result
func (c *E2eCLI) RunDockerComposeCmd(args ...string) *icmd.Result {
if composeStandaloneMode {

then in the main:

func TestMain(m *testing.M) {
	logrus.Info("Running tests on COMPOSE_STANDALONE_MODE=true")
	composeStandaloneMode = true
	exitCode := m.Run()
	if exitCode != 0 {
		os.Exit(exitCode)
	}
	logrus.Info("Running tests on COMPOSE_STANDALONE_MODE=false")
	composeStandaloneMode = false
	os.Exit(m.Run())
}

We don’t really need an env var, do we?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice this can be set as a go test argument so that we can parallelize run on CI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if parallelising this would be possible...
The e2e tests open ports and maybe that would conflict with it's twin test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't see a good way to run one specific test (by passing E2E_TEST environment variable) on one specific mode and not having to replicate Makefile targets or something like that...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what I had in mind: two targets, two parallel GHA workfows
never mind

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s a Makefile? 😬

It’s already possible to focus tests out of the box using -run with go test: https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks
(I don’t have make on my machines)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mat007 Yep. And that's what the Makefile does. The thing is, when targeting a specific test TestMain doesn't run. So that would execute just one of the modes (the default one).
Then the question is... Do we separate the execution of the 2 modes into different runs or leave it in the same (like it's implemented in this PR).
Now I'm leaning to having 2 runs in the GHA as @ndeloof said cuz that would be easier to run in local. Cuz I don't want to run the 2 things all the time I test locally. But for the CI that's important. And also that can be run in parallel...

@ulyssessouza
Copy link
Contributor Author

@mat007 PTAL

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
Copy link
Member

@mat007 mat007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ulyssessouza ulyssessouza merged commit 40bca10 into docker:v2 Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants