packer_test: split BuildPackerPlugin in build/get

The interface for building a plugin through the test suite was
confusing, as it would build the plugin and return its path, cache the
path for the version built, and return the path regardless if something
was built or not.

While in the current state this is harmless as builds are idempotent,
since the state of the plugin package/module does not change, this will
in the future as we introduce customisation techniques on the plugin's
directory and files, making this double-use potentially dangerous.

Furthermore, the current behaviour is unclear, as the function hides
that caching mechanism, which could come as a surprise for users
attempting to build a plugin for the duration of a test, while the built
plugin is linked to the test suite being run, and not the unit test
being evaluated.

Therefore this commit changes the sequence in which plugins are built
and used. Now the `CompilePlugin` function builds a plugin, and does not
return its path anymore, instead terminating the tests immediately if
they fail.
In normal test usage, a new `GetPluginPath` function is introduced,
which looks-up the path in the suite's cache, failing immediately if
invoked before the plugin is built.

With this change, it is heavily advised to build plugins when
initialising the suite, then in the tests, the GetPluginPath function
should be used to get a plugin's path for interacting with packer
commands.

packer_test/lib/plugin.go

@@ -61,9 +61,24 @@ func ExpectedInstalledName(versionStr string) string {
 		runtime.GOOS, runtime.GOARCH, ext)
 }
 
-// BuildSimplePlugin creates a plugin that essentially does nothing.
+// GetPluginPath gets the path for a pre-compiled plugin in the current test suite.
 //
-// The plugin's code is contained in a subdirectory of this, and lets us
+// The version only is needed, as the path to a compiled version of the tester
+// plugin will be returned, so it can be installed after the fact.
+//
+// If the version requested does not exist, the function will panic.
+func (ts *PackerTestSuite) GetPluginPath(t *testing.T, version string) string {
+	path, ok := ts.compiledPlugins.Load(version)
+	if !ok {
+		t.Fatalf("tester plugin in version %q was not built, either build it during suite init, or with BuildTestPlugin", version)
+	}
+
+	return path.(string)
+}
+
+// CompilePlugin builds a tester plugin with the specified version.
+//
+// The plugin's code is contained in a subdirectory of this file, and lets us
 // change the attributes of the plugin binary itself, like the SDK version,
 // the plugin's version, etc.
 //
@@ -73,16 +88,24 @@ func ExpectedInstalledName(versionStr string) string {
 //
 // The path to the plugin is returned, it won't be removed automatically
 // though, deletion is the caller's responsibility.
-func (ts *PackerTestSuite) BuildSimplePlugin(versionString string, t *testing.T) string {
-	// Only build plugin binary if not already done beforehand
-	path, ok := ts.compiledPlugins.Load(versionString)
-	if ok {
-		return path.(string)
+//
+// Note: each tester plugin may only be compiled once for a specific version in
+// a test suite. The version may include core (mandatory), pre-release and
+// metadata. Unlike Packer core, metadata does matter for the version being built.
+func (ts *PackerTestSuite) CompilePlugin(t *testing.T, versionString string) {
+	// Fail to build plugin if already built.
+	//
+	// Especially with customisations being a thing, relying on cache to get and
+	// build a plugin at once means that the function is not idempotent anymore,
+	// and therefore we cannot rely on it being called twice and producing the
+	// same result, so we forbid it.
+	if _, ok := ts.compiledPlugins.Load(versionString); ok {
+		t.Fatalf("plugin version %q was already built, use GetTestPlugin instead", versionString)
 	}
 
 	v := version.Must(version.NewSemver(versionString))
 
-	t.Logf("Building plugin in version %v", v)
+	t.Logf("Building tester plugin in version %v", v)
 
 	testDir, err := currentDir()
 	if err != nil {
@@ -99,8 +122,6 @@ func (ts *PackerTestSuite) BuildSimplePlugin(versionString string, t *testing.T)
 	}
 
 	ts.compiledPlugins.Store(v.String(), outBin)
-
-	return outBin
 }
 
 // MakePluginDir installs a list of plugins into a temporary directory and returns its path
@@ -112,8 +133,8 @@ func (ts *PackerTestSuite) BuildSimplePlugin(versionString string, t *testing.T)
 func (ts *PackerTestSuite) MakePluginDir(pluginVersions ...string) (pluginTempDir string, cleanup func()) {
 	t := ts.T()
 
-	for _, ver := range pluginVersions {
-		ts.BuildSimplePlugin(ver, t)
+	for _, version := range pluginVersions {
+		_ = ts.GetPluginPath(t, version)
 	}
 
 	var err error
@@ -133,11 +154,8 @@ func (ts *PackerTestSuite) MakePluginDir(pluginVersions ...string) (pluginTempDi
 	}
 
 	for _, pluginVersion := range pluginVersions {
-		path, ok := ts.compiledPlugins.Load(pluginVersion)
-		if !ok {
-			err = fmt.Errorf("failed to get path to version %q, was it compiled?", pluginVersion)
-		}
-		cmd := ts.PackerCommand().SetArgs("plugins", "install", "--path", path.(string), "github.com/hashicorp/tester").AddEnv("PACKER_PLUGIN_PATH", pluginTempDir)
+		path := ts.GetPluginPath(t, pluginVersion)
+		cmd := ts.PackerCommand().SetArgs("plugins", "install", "--path", path, "github.com/hashicorp/tester").AddEnv("PACKER_PLUGIN_PATH", pluginTempDir)
 		cmd.Assert(MustSucceed())
 		out, stderr, cmdErr := cmd.Run()
 		if cmdErr != nil {

packer_test/lib/suite.go

@@ -34,7 +34,7 @@ func (ts *PackerTestSuite) buildPluginVersion(waitgroup *sync.WaitGroup, version
 	waitgroup.Add(1)
 	go func() {
 		defer waitgroup.Done()
-		ts.BuildSimplePlugin(versionString, t)
+		ts.CompilePlugin(t, versionString)
 	}()
 }
 

packer_test/plugin_tests/init_test.go

@@ -55,7 +55,7 @@ func (ts *PackerPluginTestSuite) TestPackerInitWithNonGithubSource() {
 
 	ts.Run("manually install plugin to the expected source", func() {
 		ts.PackerCommand().UsePluginDir(pluginPath).
-			SetArgs("plugins", "install", "--path", ts.BuildSimplePlugin("1.0.10", ts.T()), "hubgit.com/hashicorp/tester").
+			SetArgs("plugins", "install", "--path", ts.GetPluginPath(ts.T(), "1.0.10"), "hubgit.com/hashicorp/tester").
 			Assert(lib.MustSucceed(), lib.Grep("packer-plugin-tester_v1.0.10", lib.GrepStdout))
 	})
 

packer_test/plugin_tests/install_test.go

@@ -8,7 +8,7 @@ func (ts *PackerPluginTestSuite) TestInstallPluginWithMetadata() {
 
 	ts.Run("install plugin with metadata in version", func() {
 		ts.PackerCommand().UsePluginDir(tempPluginDir).
-			SetArgs("plugins", "install", "--path", ts.BuildSimplePlugin("1.0.0+metadata", ts.T()), "github.com/hashicorp/tester").
+			SetArgs("plugins", "install", "--path", ts.GetPluginPath(ts.T(), "1.0.0+metadata"), "github.com/hashicorp/tester").
 			Assert(lib.MustSucceed(), lib.Grep("Successfully installed plugin", lib.GrepStdout))
 	})
 
@@ -37,13 +37,13 @@ func (ts *PackerPluginTestSuite) TestInstallPluginWithPath() {
 
 	ts.Run("install plugin with pre-release only", func() {
 		ts.PackerCommand().UsePluginDir(tempPluginDir).
-			SetArgs("plugins", "install", "--path", ts.BuildSimplePlugin("1.0.0-dev", ts.T()), "github.com/hashicorp/tester").
+			SetArgs("plugins", "install", "--path", ts.GetPluginPath(ts.T(), "1.0.0-dev"), "github.com/hashicorp/tester").
 			Assert(lib.MustSucceed(), lib.Grep("Successfully installed plugin", lib.GrepStdout))
 	})
 
 	ts.Run("install same plugin with pre-release + metadata", func() {
 		ts.PackerCommand().UsePluginDir(tempPluginDir).
-			SetArgs("plugins", "install", "--path", ts.BuildSimplePlugin("1.0.0-dev+metadata", ts.T()), "github.com/hashicorp/tester").
+			SetArgs("plugins", "install", "--path", ts.GetPluginPath(ts.T(), "1.0.0-dev+metadata"), "github.com/hashicorp/tester").
 			Assert(lib.MustSucceed(), lib.Grep("Successfully installed plugin", lib.GrepStdout))
 	})
 
@@ -58,7 +58,7 @@ func (ts *PackerPluginTestSuite) TestInstallPluginWithPath() {
 }
 
 func (ts *PackerPluginTestSuite) TestInstallPluginPrerelease() {
-	pluginPath := ts.BuildSimplePlugin("1.0.1-alpha1", ts.T())
+	pluginPath := ts.GetPluginPath(ts.T(), "1.0.1-alpha1")
 
 	pluginDir, cleanup := ts.MakePluginDir()
 	defer cleanup()

packer_test/plugin_tests/loading_test.go

@@ -51,7 +51,7 @@ func (ts *PackerPluginTestSuite) TestLoadWithLegacyPluginName() {
 	pluginDir, cleanup := ts.MakePluginDir()
 	defer cleanup()
 
-	plugin := ts.BuildSimplePlugin("1.0.10", ts.T())
+	plugin := ts.GetPluginPath(ts.T(), "1.0.10")
 
 	lib.CopyFile(ts.T(), filepath.Join(pluginDir, "packer-plugin-tester"), plugin)
 
@@ -96,7 +96,7 @@ func (ts *PackerPluginTestSuite) TestLoadWithLegacyPluginName() {
 }
 
 func (ts *PackerPluginTestSuite) TestLoadWithSHAMismatches() {
-	plugin := ts.BuildSimplePlugin("1.0.10", ts.T())
+	plugin := ts.GetPluginPath(ts.T(), "1.0.10")
 
 	ts.Run("move plugin with right name, but no SHA256SUM, should reject", func() {
 		pluginDir, cleanup := ts.MakePluginDir("1.0.9")
@@ -161,7 +161,7 @@ func (ts *PackerPluginTestSuite) TestInstallNonCanonicalPluginVersion() {
 
 	lib.ManualPluginInstall(ts.T(),
 		filepath.Join(pluginPath, "github.com", "hashicorp", "tester"),
-		ts.BuildSimplePlugin("1.0.10", ts.T()),
+		ts.GetPluginPath(ts.T(), "1.0.10"),
 		"001.00.010")
 
 	ts.Run("try listing plugins with non-canonical version installed - report none", func() {
@@ -179,7 +179,7 @@ func (ts *PackerPluginTestSuite) TestLoadPluginWithMetadataInName() {
 
 	lib.ManualPluginInstall(ts.T(),
 		filepath.Join(pluginPath, "github.com", "hashicorp", "tester"),
-		ts.BuildSimplePlugin("1.0.10+metadata", ts.T()),
+		ts.GetPluginPath(ts.T(), "1.0.10+metadata"),
 		"1.0.10+metadata")
 
 	ts.Run("try listing plugins with metadata in name - report none", func() {