update after review feedback

* return scanner error also
* use testing TempDir
* split test method
* test various error returns also

2 files changed, 64 insertions, 19 deletions

diff --git a/client/go/vespa/load_env.go b/client/go/vespa/load_env.go
index dcd14131842..a0c127ca920 100644
--- a/client/go/vespa/load_env.go
+++ b/client/go/vespa/load_env.go
@@ -53,6 +53,9 @@ func LoadDefaultEnv() error {
 			err = fmt.Errorf("unknown action '%s'", action)
 		}
 	}
+	if err == nil {
+		err = scanner.Err()
+	}
 	return err
 }
 
diff --git a/client/go/vespa/load_env_test.go b/client/go/vespa/load_env_test.go
index 17e01f23fea..c5b42cae161 100644
--- a/client/go/vespa/load_env_test.go
+++ b/client/go/vespa/load_env_test.go
@@ -2,57 +2,99 @@
 package vespa
 
 import (
-	"fmt"
 	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func TestLoadEnv(t *testing.T) {
-	// setup
-	tmp := os.TempDir() + "/load_env_test.tmp"
+func setup(t *testing.T, contents string) {
+	tmp := t.TempDir() + "/load_env_test.tmp"
 	vdir := tmp + "/vespa"
 	cdir := vdir + "/conf/vespa"
 	envf := cdir + "/default-env.txt"
 	err := os.MkdirAll(cdir, 0755)
-	defer os.RemoveAll(tmp)
 	assert.Nil(t, err)
-	fmt.Println("vdir:", vdir)
 	os.Setenv("VESPA_HOME", vdir)
+	err = os.WriteFile(envf, []byte(contents), 0644)
+	assert.Nil(t, err)
+}
+
+func TestLoadEnvSimple(t *testing.T) {
 	os.Setenv("VESPA_FOO", "was foo")
 	os.Setenv("VESPA_BAR", "was bar")
 	os.Setenv("VESPA_FOOBAR", "foobar")
 	os.Unsetenv("VESPA_QUUX")
-	contents := `# vespa env vars file
+	setup(t, `
+# vespa env vars file
 override VESPA_FOO "new foo"
+
 fallback VESPA_BAR "new bar"
 fallback VESPA_QUUX "new quux"
+
 unset VESPA_FOOBAR
+`)
+	// run it
+	err := LoadDefaultEnv()
+	assert.Nil(t, err)
+	// check results
+	assert.Equal(t, os.Getenv("VESPA_FOO"), "new foo")
+	assert.Equal(t, os.Getenv("VESPA_BAR"), "was bar")
+	assert.Equal(t, os.Getenv("VESPA_FOOBAR"), "")
+	assert.Equal(t, os.Getenv("VESPA_QUUX"), "new quux")
+	_, present := os.LookupEnv("VESPA_FOOBAR")
+	assert.Equal(t, present, false)
+}
+
+func TestLoadEnvWhiteSpace(t *testing.T) {
+	// note trailing whitespace below!
+	setup(t, `
+# vespa env vars file
 override VESPA_V1 v1
  override  VESPA_V2  v2 
 override VESPA_V3 spaced v3 v3
 override VESPA_V4 " quoted spaced "
-
-some junk here
 override VESPA_V5 v5
-`
-	err = os.WriteFile(envf, []byte(contents), 0644)
-	assert.Nil(t, err)
+`)
 	// run it
-	err = LoadDefaultEnv()
+	err := LoadDefaultEnv()
+	assert.Nil(t, err)
 	// check results
-	assert.Equal(t, os.Getenv("VESPA_FOO"), "new foo")
-	assert.Equal(t, os.Getenv("VESPA_BAR"), "was bar")
-	assert.Equal(t, os.Getenv("VESPA_FOOBAR"), "")
-	assert.Equal(t, os.Getenv("VESPA_QUUX"), "new quux")
 	assert.Equal(t, os.Getenv("VESPA_V1"), "v1")
 	assert.Equal(t, os.Getenv("VESPA_V2"), "v2")
 	assert.Equal(t, os.Getenv("VESPA_V3"), "spaced v3 v3")
 	assert.Equal(t, os.Getenv("VESPA_V4"), " quoted spaced ")
 	assert.Equal(t, os.Getenv("VESPA_V5"), "v5")
-	_, present := os.LookupEnv("VESPA_FOOBAR")
-	assert.Equal(t, present, false)
+}
+
+func TestLoadEnvBadAction(t *testing.T) {
+	setup(t, `
+# vespa env vars file
+override VESPA_V1 v1
+some junk here
+override VESPA_V2 v2
+`)
+	// run it
+	err := LoadDefaultEnv()
+	// check results
+	assert.Equal(t, os.Getenv("VESPA_V1"), "v1")
+	assert.Equal(t, os.Getenv("VESPA_V2"), "v2")
 	assert.NotNil(t, err)
 	assert.Equal(t, err.Error(), "unknown action 'some'")
 }
+
+func TestLoadEnvBadVar(t *testing.T) {
+	setup(t, `
+# vespa env vars file
+override VESPA_V1 v1
+override .A foobar
+override VESPA_V2 v2
+`)
+	// run it
+	err := LoadDefaultEnv()
+	// check results
+	assert.Equal(t, os.Getenv("VESPA_V1"), "v1")
+	assert.Equal(t, os.Getenv("VESPA_V2"), "v2")
+	assert.NotNil(t, err)
+	assert.Equal(t, err.Error(), "Not a valid environment variable name: '.A'")
+}