Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cleanup errchecking in tests #3040

Merged
merged 37 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
8c16eda
add errcheck linting
mkcp Sep 11, 2024
430467e
linter ignore some intentionally unhandled errs
mkcp Sep 12, 2024
2e093b1
handle some env var set and unset errors in tests
mkcp Sep 12, 2024
0e9405e
make some notes in cmd/destroy.go for later
mkcp Sep 12, 2024
5ad8530
direct handle some deferred cleanups to add direct error handling
mkcp Sep 12, 2024
23663e4
capture errors from cmd/root
mkcp Sep 13, 2024
7645357
capture errors on src/config/config.go. checking ci
mkcp Sep 13, 2024
4b5c794
fix: ensure we capture and propagate errors in src/internal
mkcp Sep 25, 2024
849680d
fix: propagate or log unhandled errors in src/cmd
mkcp Sep 25, 2024
433aacd
fix: check for or intentionally ignore some dangling errors in tests
mkcp Sep 25, 2024
b579634
fix: use RemoveAll not remove so it doesnt error every time
mkcp Sep 25, 2024
7b418d6
fix: disabling the err catch to see if it's something i changed
mkcp Sep 25, 2024
e8e4ee9
fix: should fix the directory not empty panic
mkcp Sep 25, 2024
73cb6a4
fix: see if CI is flaking with the original tests. probably just need…
mkcp Sep 25, 2024
81b8c05
fix: these tests should clean up properly and not error
mkcp Sep 25, 2024
cf43754
fix: actually cleaning up the test with RemovalAll seems to cause a p…
mkcp Sep 25, 2024
2f1bd63
fix: reintroduce the ext_in_cluster test error catches
mkcp Sep 25, 2024
f377bb0
fix: missed a spot
mkcp Sep 25, 2024
4d87c74
fix: e2e 24 - can we assert that this error exists or do we have to i…
mkcp Sep 25, 2024
1676109
fix: address some fixmes before merge
mkcp Sep 25, 2024
8e36039
fix: does this work now?
mkcp Sep 25, 2024
76a6665
fix: handle and propagate errors in pkg/utils
mkcp Sep 25, 2024
c6e592d
fix: propagate any errors in ColorPrintYAML
mkcp Sep 25, 2024
752f0ff
fix: propagate any errors in src/pkg/message
mkcp Sep 25, 2024
cbb192e
fix: propagate any errors in src/pkg/variables
mkcp Sep 25, 2024
9eb3ef0
fix: propagate any errors in src/pkg/layout
mkcp Sep 25, 2024
0e6ef61
fix: srcfile is already closed and the error handled, no need to defe…
mkcp Sep 25, 2024
7879eb8
fix: ensure errors from deferred resource cleanup are passed back to …
mkcp Sep 26, 2024
1c471d2
chore: undo linter rule for CI so we can merge incremental changes
mkcp Sep 26, 2024
a62fba9
chore: undo the other linter rule
mkcp Sep 26, 2024
3ddd3dc
chore: undo non-testing changes
mkcp Sep 26, 2024
20241aa
fix: remove nolints for now to make ci happy. we'll have to readd later
mkcp Sep 26, 2024
9d5eced
fix: ensure we're defering resource cleanup in the tests
mkcp Sep 26, 2024
93417ee
chore: mark todos where we'll need to nolint later
mkcp Sep 26, 2024
cc6b253
chore: ensure NoError e2e.CleanFiles and pass thru testing.T
mkcp Sep 30, 2024
838e1e0
chore: use const
mkcp Sep 30, 2024
e23dd79
chore: ensure we don't try to close a nil response body
mkcp Sep 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/test/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ package test

import (
"bufio"
"errors"
"fmt"
"log/slog"
"os"
"regexp"
"runtime"
Expand Down Expand Up @@ -53,13 +55,16 @@ func GetCLIName() string {
}

// Zarf executes a Zarf command.
func (e2e *ZarfE2ETest) Zarf(t *testing.T, args ...string) (string, string, error) {
func (e2e *ZarfE2ETest) Zarf(t *testing.T, args ...string) (_ string, _ string, err error) {
if !slices.Contains(args, "--tmpdir") && !slices.Contains(args, "tools") {
tmpdir, err := os.MkdirTemp("", "zarf-")
if err != nil {
return "", "", err
}
defer os.RemoveAll(tmpdir)
defer func(path string) {
errRemove := os.RemoveAll(path)
err = errors.Join(err, errRemove)
}(tmpdir)
args = append(args, "--tmpdir", tmpdir)
}
if !slices.Contains(args, "--zarf-cache") && !slices.Contains(args, "tools") && os.Getenv("CI") == "true" {
Expand All @@ -74,7 +79,10 @@ func (e2e *ZarfE2ETest) Zarf(t *testing.T, args ...string) (string, string, erro
return "", "", err
}
args = append(args, "--zarf-cache", cacheDir)
defer os.RemoveAll(cacheDir)
defer func(path string) {
errRemove := os.RemoveAll(path)
err = errors.Join(err, errRemove)
}(cacheDir)
}
return exec.CmdWithTesting(t, exec.PrintCfg(), e2e.ZarfBinPath, args...)
}
Expand All @@ -89,7 +97,10 @@ func (e2e *ZarfE2ETest) Kubectl(t *testing.T, args ...string) (string, string, e
// CleanFiles removes files and directories that have been created during the test.
func (e2e *ZarfE2ETest) CleanFiles(files ...string) {
for _, file := range files {
_ = os.RemoveAll(file)
err := os.RemoveAll(file)
AustinAbro321 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
slog.Debug("Unable to cleanup files", "files", files, "error", err)
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/test/e2e/00_use_cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ func TestUseCLI(t *testing.T) {
t.Run("changing log level", func(t *testing.T) {
t.Parallel()
// Test that changing the log level actually applies the requested level
_, stdErr, _ := e2e.Zarf(t, "internal", "crc32", "zarf", "--log-level=debug")
_, stdErr, err := e2e.Zarf(t, "internal", "crc32", "zarf", "--log-level=debug")
require.NoError(t, err)
expectedOutString := "Log level set to debug"
require.Contains(t, stdErr, expectedOutString, "The log level should be changed to 'debug'")
})
Expand Down
6 changes: 4 additions & 2 deletions src/test/e2e/12_lint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@ func TestLint(t *testing.T) {

testPackagePath := filepath.Join("src", "test", "packages", "12-lint")
configPath := filepath.Join(testPackagePath, "zarf-config.toml")
os.Setenv("ZARF_CONFIG", configPath)
osSetErr := os.Setenv("ZARF_CONFIG", configPath)
require.NoError(t, osSetErr, "Unable to set ZARF_CONFIG")
_, stderr, err := e2e.Zarf(t, "dev", "lint", testPackagePath, "-f", "good-flavor")
os.Unsetenv("ZARF_CONFIG")
osUnsetErr := os.Unsetenv("ZARF_CONFIG")
require.NoError(t, osUnsetErr, "Unable to cleanup ZARF_CONFIG")
require.Error(t, err, "Require an exit code since there was warnings / errors")
strippedStderr := e2e.StripMessageFormatting(stderr)

Expand Down
79 changes: 44 additions & 35 deletions src/test/e2e/14_oci_compose_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,45 +137,54 @@ func (suite *PublishCopySkeletonSuite) Test_2_FilePaths() {
}

for _, pkgTar := range pkgTars {
var pkg v1alpha1.ZarfPackage

unpacked := strings.TrimSuffix(pkgTar, ".tar.zst")
defer os.RemoveAll(unpacked)
defer os.RemoveAll(pkgTar)
_, _, err := e2e.Zarf(suite.T(), "tools", "archiver", "decompress", pkgTar, unpacked, "--unarchive-all")
suite.NoError(err)
suite.DirExists(unpacked)

// Verify skeleton contains kustomize-generated manifests.
if strings.HasSuffix(pkgTar, "zarf-package-test-compose-package-skeleton-0.0.1.tar.zst") {
kustomizeGeneratedManifests := []string{
"kustomization-connect-service-0.yaml",
"kustomization-connect-service-1.yaml",
"kustomization-connect-service-two-0.yaml",
}
manifestDir := filepath.Join(unpacked, "components", "test-compose-package", "manifests")
for _, manifest := range kustomizeGeneratedManifests {
manifestPath := filepath.Join(manifestDir, manifest)
suite.FileExists(manifestPath, "expected to find kustomize-generated manifest: %q", manifestPath)
var configMap corev1.ConfigMap
err := utils.ReadYaml(manifestPath, &configMap)
suite.NoError(err)
suite.Equal("ConfigMap", configMap.Kind, "expected manifest %q to be of kind ConfigMap", manifestPath)
// Wrap in a fn to ensure our defers cleanup resources on each iteration
mkcp marked this conversation as resolved.
Show resolved Hide resolved
func() {
var pkg v1alpha1.ZarfPackage

unpacked := strings.TrimSuffix(pkgTar, ".tar.zst")
_, _, err := e2e.Zarf(suite.T(), "tools", "archiver", "decompress", pkgTar, unpacked, "--unarchive-all")
suite.NoError(err)
suite.DirExists(unpacked)

// Cleanup resources
defer func() {
suite.NoError(os.RemoveAll(unpacked))
}()
defer func() {
suite.NoError(os.RemoveAll(pkgTar))
}()

// Verify skeleton contains kustomize-generated manifests.
if strings.HasSuffix(pkgTar, "zarf-package-test-compose-package-skeleton-0.0.1.tar.zst") {
kustomizeGeneratedManifests := []string{
"kustomization-connect-service-0.yaml",
"kustomization-connect-service-1.yaml",
"kustomization-connect-service-two-0.yaml",
}
manifestDir := filepath.Join(unpacked, "components", "test-compose-package", "manifests")
for _, manifest := range kustomizeGeneratedManifests {
manifestPath := filepath.Join(manifestDir, manifest)
suite.FileExists(manifestPath, "expected to find kustomize-generated manifest: %q", manifestPath)
var configMap corev1.ConfigMap
err := utils.ReadYaml(manifestPath, &configMap)
suite.NoError(err)
suite.Equal("ConfigMap", configMap.Kind, "expected manifest %q to be of kind ConfigMap", manifestPath)
}
}
}

err = utils.ReadYaml(filepath.Join(unpacked, layout.ZarfYAML), &pkg)
suite.NoError(err)
suite.NotNil(pkg)
err = utils.ReadYaml(filepath.Join(unpacked, layout.ZarfYAML), &pkg)
suite.NoError(err)
suite.NotNil(pkg)

components := pkg.Components
suite.NotNil(components)
components := pkg.Components
suite.NotNil(components)

isSkeleton := false
if strings.Contains(pkgTar, "-skeleton-") {
isSkeleton = true
}
suite.verifyComponentPaths(unpacked, components, isSkeleton)
isSkeleton := false
if strings.Contains(pkgTar, "-skeleton-") {
isSkeleton = true
}
suite.verifyComponentPaths(unpacked, components, isSkeleton)
}()
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/e2e/20_zarf_init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@ func TestZarfInit(t *testing.T) {
verifyZarfServiceLabels(t)

// Special sizing-hacking for reducing resources where Kind + CI eats a lot of free cycles (ignore errors)
_, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "kube-system", "coredns", "--replicas=1")
_, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "zarf", "agent-hook", "--replicas=1")
_, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "kube-system", "coredns", "--replicas=1") // TODO(mkcp): intentionally ignored, mark nolint
_, _, _ = e2e.Kubectl(t, "scale", "deploy", "-n", "zarf", "agent-hook", "--replicas=1") // TODO(mkcp): intentionally ignored, mark nolint
}

func checkLogForSensitiveState(t *testing.T, logText string, zarfState types.ZarfState) {
Expand Down
4 changes: 3 additions & 1 deletion src/test/e2e/21_connect_creds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,12 @@ func TestMetrics(t *testing.T) {
client := &http.Client{Transport: tr}
httpsEndpoint := strings.ReplaceAll(tunnel.HTTPEndpoint(), "http", "https")
resp, err := client.Get(httpsEndpoint + "/metrics")
defer func() {
require.NoError(t, resp.Body.Close())
mkcp marked this conversation as resolved.
Show resolved Hide resolved
}()
if err != nil {
t.Fatal(err)
}
defer resp.Body.Close()

// Read the response body
body, err := io.ReadAll(resp.Body)
Expand Down
6 changes: 4 additions & 2 deletions src/test/e2e/24_variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ func TestVariables(t *testing.T) {
require.Contains(t, stdErr, "", expectedOutString)

// Test that not specifying a prompted variable results in an error
_, stdErr, _ = e2e.Zarf(t, "package", "deploy", path, "--confirm")
_, stdErr, err = e2e.Zarf(t, "package", "deploy", path, "--confirm")
expectedOutString = "variable 'SITE_NAME' must be '--set' when using the '--confirm' flag"
require.Error(t, err)
require.Contains(t, stdErr, "", expectedOutString)

// Test that specifying an invalid variable value results in an error
Expand Down Expand Up @@ -73,7 +74,8 @@ func TestVariables(t *testing.T) {
require.Contains(t, string(outputTF), "unicorn-land")

// Verify the configmap was properly templated
kubectlOut, _, _ := e2e.Kubectl(t, "-n", "nginx", "get", "configmap", "nginx-configmap", "-o", "jsonpath='{.data.index\\.html}' ")
kubectlOut, _, err := e2e.Kubectl(t, "-n", "nginx", "get", "configmap", "nginx-configmap", "-o", "jsonpath='{.data.index\\.html}' ")
require.NoError(t, err, "unable to get nginx configmap")
// OPTIONAL_FOOTER should remain unset because it was not set during deploy
require.Contains(t, string(kubectlOut), "</pre>\n \n </body>")
// STYLE should take the default value
Expand Down
8 changes: 5 additions & 3 deletions src/test/e2e/25_helm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ func testHelmEscaping(t *testing.T) {
require.NoError(t, err, stdOut, stdErr)

// Verify the configmap was deployed, escaped, and contains all of its data
kubectlOut, _ := exec.Command("kubectl", "-n", "default", "describe", "cm", "dont-template-me").Output()
kubectlOut, err := exec.Command("kubectl", "-n", "default", "describe", "cm", "dont-template-me").Output()
require.NoError(t, err, "unable to describe configmap")
require.Contains(t, string(kubectlOut), `alert: OOMKilled {{ "{{ \"random.Values\" }}" }}`)
require.Contains(t, string(kubectlOut), "backtick1: \"content with backticks `some random things`\"")
require.Contains(t, string(kubectlOut), "backtick2: \"nested templating with backticks {{` random.Values `}}\"")
Expand Down Expand Up @@ -175,8 +176,9 @@ func testHelmAdoption(t *testing.T) {
deploymentManifest := "src/test/packages/25-manifest-adoption/deployment.yaml"

// Deploy dos-games manually into the cluster without Zarf
kubectlOut, _, _ := e2e.Kubectl(t, "apply", "-f", deploymentManifest)
require.Contains(t, string(kubectlOut), "deployment.apps/game created")
kubectlOut, _, err := e2e.Kubectl(t, "apply", "-f", deploymentManifest)
require.NoError(t, err, "unable to apply", "deploymentManifest", deploymentManifest)
require.Contains(t, kubectlOut, "deployment.apps/game created")

// Deploy dos-games into the cluster with Zarf
stdOut, stdErr, err := e2e.Zarf(t, "package", "deploy", packagePath, "--confirm", "--adopt-existing-resources")
Expand Down
2 changes: 1 addition & 1 deletion src/test/e2e/28_wait_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestNoWait(t *testing.T) {
case <-time.After(30 * time.Second):
t.Error("Timeout waiting for zarf deploy (it tried to wait)")
t.Log("Removing hanging namespace...")
_, _, _ = e2e.Kubectl(t, "delete", "namespace", "no-wait", "--force=true", "--wait=false", "--grace-period=0")
_, _, _ = e2e.Kubectl(t, "delete", "namespace", "no-wait", "--force=true", "--wait=false", "--grace-period=0") // TODO(mkcp): intentionally ignored, mark nolint
}
require.NoError(t, err, stdOut, stdErr)

Expand Down
35 changes: 23 additions & 12 deletions src/test/e2e/29_config_file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,24 @@ func TestConfigFile(t *testing.T) {
path = fmt.Sprintf("zarf-package-config-file-%s.tar.zst", e2e.Arch)
dir = "examples/config-file"
config = "zarf-config.toml"
envKey = "ZARF_CONFIG"
)

e2e.CleanFiles(path)

// Test the config file environment variable
t.Setenv("ZARF_CONFIG", filepath.Join(dir, config))
defer os.Unsetenv("ZARF_CONFIG")
t.Setenv(envKey, filepath.Join(dir, config))
defer func() {
require.NoError(t, os.Unsetenv(envKey))
}()
configFileTests(t, dir, path)

configFileDefaultTests(t)

stdOut, stdErr, err := e2e.Zarf(t, "package", "remove", path, "--confirm")
require.NoError(t, err, stdOut, stdErr)

// Cleanup
e2e.CleanFiles(path)
}

Expand Down Expand Up @@ -139,30 +143,37 @@ func configFileDefaultTests(t *testing.T) {
}

// Test remaining default initializers
t.Setenv("ZARF_CONFIG", filepath.Join("src", "test", "zarf-config-test.toml"))
defer os.Unsetenv("ZARF_CONFIG")
envKey := "ZARF_CONFIG"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could just set as const mayb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def, opted to keep it consistent but const was preferred

t.Setenv(envKey, filepath.Join("src", "test", "zarf-config-test.toml"))
defer func() {
require.NoError(t, os.Unsetenv(envKey))
}()

// Test global flags
stdOut, _, _ := e2e.Zarf(t, "--help")
stdOut, _, err := e2e.Zarf(t, "--help")
require.NoError(t, err)
for _, test := range globalFlags {
require.Contains(t, string(stdOut), test)
require.Contains(t, stdOut, test)
}

// Test init flags
stdOut, _, _ = e2e.Zarf(t, "init", "--help")
stdOut, _, err = e2e.Zarf(t, "init", "--help")
require.NoError(t, err)
for _, test := range initFlags {
require.Contains(t, string(stdOut), test)
require.Contains(t, stdOut, test)
}

// Test package create flags
stdOut, _, _ = e2e.Zarf(t, "package", "create", "--help")
stdOut, _, err = e2e.Zarf(t, "package", "create", "--help")
require.NoError(t, err)
for _, test := range packageCreateFlags {
require.Contains(t, string(stdOut), test)
require.Contains(t, stdOut, test)
}

// Test package deploy flags
stdOut, _, _ = e2e.Zarf(t, "package", "deploy", "--help")
stdOut, _, err = e2e.Zarf(t, "package", "deploy", "--help")
require.NoError(t, err)
for _, test := range packageDeployFlags {
require.Contains(t, string(stdOut), test)
require.Contains(t, stdOut, test)
}
}
3 changes: 2 additions & 1 deletion src/test/external/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ func createPodInfoPackageWithInsecureSources(t *testing.T, temp string) {
require.NoError(t, err, "unable to yq edit helm source")
err = exec.CmdWithPrint(zarfBinPath, "tools", "yq", "eval", ".spec.insecure = true", "-i", filepath.Join(temp, "oci", "podinfo-source.yaml"))
require.NoError(t, err, "unable to yq edit oci source")
exec.CmdWithPrint(zarfBinPath, "package", "create", temp, "--confirm", "--output", temp)
err = exec.CmdWithPrint(zarfBinPath, "package", "create", temp, "--confirm", "--output", temp)
require.NoError(t, err, "unable to create package")
}

func verifyWaitSuccess(t *testing.T, timeoutMinutes time.Duration, cmd string, args []string, condition string, onTimeout string) bool {
Expand Down
6 changes: 5 additions & 1 deletion src/test/external/ext_in_cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,14 @@ func (suite *ExtInClusterTestSuite) Test_1_Deploy() {
// Use Zarf to initialize the cluster
initArgs := []string{"init", "--confirm"}
initArgs = append(initArgs, inClusterInitCredentialArgs...)

err := exec.CmdWithPrint(zarfBinPath, initArgs...)
suite.NoError(err, "unable to initialize the k8s server with zarf")

temp := suite.T().TempDir()
defer os.Remove(temp)
defer func() {
suite.NoError(os.RemoveAll(temp), "failed to clean up tempdir")
}()
createPodInfoPackageWithInsecureSources(suite.T(), temp)

// Deploy the flux example package
Expand Down
Loading