Add run integration test

[jgoldstein46]

• Adds an integration test for the vers run command.
• Steps to test:

1. As noted in the README, add the necessary environment variables.
2. cd test && go test -v
   -> Verify cluster started and torn down.

[asebexen]

Just to confirm: we want a .env in the project root, even though we're cd'ing into the test directory? (See later comments re: setting up a temp directory.)

[jgoldstein46]

Yes! I noted this above the .env

[asebexen]

nit: Generally, prefer path resolution libraries over generating arbitrary path strings. It's more explicit, handles common errors like having double forward slashes, and handles this behavior in a platform-independent manner (escaping characters that may need escaping, using backslashes on Windows, etc.)
https://pkg.go.dev/path

[jgoldstein46]

Thanks for mentioning this! I believe the filepath package was what you meant -- this is a better fit for handling os paths. Updated all references path references to use this.

[asebexen]

Assumes variable is set and doesn't provide feedback if not. (Incidentally, this variable being unset is why the test on GH is currently failing.) Maybe it should attempt to exec go without a fully qualified path if this isn't set? Or maybe it should just throw a more helpful error.

[jgoldstein46]

Added a check for all environment variables being set. If not all are set, the test will skip and log which are missing.

[jgoldstein46]

Execing go without a fully qualified path has not been working for me because of the way the exec package works. I believe it expects a full path to your go executable -- it's not like a shell with a PATH environment variable.

[asebexen]

Note that using go install will install it to the user's Go bin path, usually at ~/go/bin. Recommend creating a temporary install directory (In /tmp maybe?) and installing there.

mkdir -p /tmp/vers-cli-test-abc123
GOBIN=/tmp/vers-cli-test-abc123 go install $PROJECT_ROOT/cmd/vers

Remember then to use the temp directory as the path to the vers binary. This eliminates the need for the GO_INSTALL_PATH var, since you can use a return value from your createTempInstallDirectory() function. You can additionally copy the vers.toml and any necessary stuff here, this way we can isolate the test environment, rather than mutating or potentially relying on something in the host environment.

[jgoldstein46]

Good idea, I updated the test to create the temp install directory, and move the test vers.toml there as well.

[asebexen]

If the test fails immediately due to a mismatch in expected output (rather than due to an actual error), the new cluster will not be cleaned up properly.

[jgoldstein46]

I was using testing.Errorf() for validating the output, which causes the test to fail, but allows execution to continue. To make it obvious if there was an issue killing the cluster, I just added another error logging statement so that whoever is running the test will know!

[asebexen]

As a nitpick, I'd rather see a cluster ID passed into this function. validateOutput, for example, could return an extracted cluster ID since it's already doing a regexp validation.

[jgoldstein46]

I think that ID extraction should be done in a separate function, since it uses a slightly looser regex to extract the ID. I moved it to a separate function, and now pass in the extracted ID to the kill cluster command. This separate function + logging also makes it more obvious when the cluster ID cannot be extracted.

[asebexen]

nit: extra parentheses

[asebexen]

Question: Is stdout defined if err is non-nil? I'm very used to seeing immediate error checks in Go, since I tend to assume that the first value may be nil if the error is non-nil, which will cause a segfault due to uninitialized/nil pointer dereference.

[jgoldstein46]

Looking at the source code, cmd.Output() will return stdout even if stderr is not nil. I added a check in the case that standard out is already defined and cmd.Output() exits early -- in which case it will return nil for standard out and we want to avoid the segfault.

// Output runs the command and returns its standard output.
// Any returned error will usually be of type [*ExitError].
// If c.Stderr was nil and the returned error is of type
// [*ExitError], Output populates the Stderr field of the
// returned error.
func (c *Cmd) Output() ([]byte, error) {
	if c.Stdout != nil {
		return nil, errors.New("exec: Stdout already set")
	}
	var stdout bytes.Buffer
	c.Stdout = &stdout

	captureErr := c.Stderr == nil
	if captureErr {
		c.Stderr = &prefixSuffixSaver{N: 32 << 10}
	}

	err := c.Run()
	if err != nil && captureErr {
		if ee, ok := err.(*ExitError); ok {
			ee.Stderr = c.Stderr.(*prefixSuffixSaver).Bytes()
		}
	}
	return stdout.Bytes(), err
}

[asebexen]

What an interesting file name hahaha! Afaict this is unused though. What format is this? It's like. Almost some kind of regexp, but not, because it contains unescaped periods.

[jgoldstein46]

Oh haha, it's plaintext. Yeah not being used because this package was a bit too brittle in that it did not allow me to compare output via regex. Removing this file for now. If you're interested in the package, you can check it out here.

Ty's friend who's building the crush cli was using it so I started to try it.

[asebexen]

Looks good! I only have three notes, but none of these are worth blocking the PR for:

• We may want to attempt to clean up the temp dir after tests are run if a flag is/isn't set. Not crucial, since temp directories should self-clean.
• Anyone running this against a dev node rather than a load balancer will have the login command fail. This seems reasonable, since the integration test might want to verify that login works. But it's just a case of like. "If you are running this against a dev node, you'll want to comment out the login() invocation, because it will fail." Chelsea does not expose the verify API token endpoint that it uses, while the LB does.
• Having to pass in the go path is a little odd; it might be the case that GO_PATH should be optional. Usually when you exec, you can let the user's PATH variable handle the heavy lifting: go test -v will try all entries in PATH until it finds go in one of them. In fact, the way I run the tests currently is GO_PATH=$(which go) go test -v, since which will return the same path that my terminal finds.

Minimal example:

// ./test1/main.go
package main

import (
	"fmt"
	"os"
	"os/exec"
)

func main() {
	var cmd = exec.Command("go", "run", "../test2/main.go")
	output, err := cmd.Output()
	if err != nil {
		if exitErr, ok := err.(*exec.ExitError); ok {
			fmt.Printf("Error: %s\n", exitErr.Stderr)
			os.Exit(exitErr.ExitCode())
		} else {
			fmt.Printf("Unrecognized error: %v\n", err)
			os.Exit(1)
		}
	}
	fmt.Printf("Success! Stdout: %s", output)
}

// ./test2/main.go
package main

import "fmt"

func main() {
	fmt.Printf("This is another main function!\n")
}

Output:

Success! Stdout: This is another main function!

Note that it's not necessary to specify the Go path. The exec.Command call spawns the command with the user's shell, which in my case is zsh. Zsh sources my .zshrc file, yadda yadda, basically my PATH variable contains /usr/local/go/bin, and that directory has the go executable, so my shell can resolve the command. GO_PATH may be optional if my shell doesn't have the path, but otherwise you can try to take advantage of it.

Super minor, if you wanna tweak it for that last bullet point feel free to, but yeah. Everything else looks good! The temp directory stuff is clean and works as expected. :)