[goldctl] Replace imgmatching.MatcherFactory interface and imgmatching.MatcherFactoryImpl struct with a new imgmatching.MakeMatcher function.
There's no longer a need to have an interface to facilitate testing, as testing can now be done with the real matcher factory.
Bug: skia:9527
Change-Id: Ic8ff82664b686d1d88b3c4a6a05f6e8f3a9f6254
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/279018
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index b76df08..3a45031 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -146,10 +146,6 @@
// auth stores the authentication method to use.
auth AuthOpt
httpClient HTTPClient
-
- // imgMatcherFactory builds an imgmatching.Matcher for the image matching algorithm specified via
- // optional keys, if any.
- imgMatcherFactory imgmatching.MatcherFactory
}
// GoldClientConfig is a config structure to configure GoldClient instances
@@ -194,11 +190,10 @@
}
ret := CloudClient{
- workDir: workDir,
- auth: authOpt,
- loadAndHashImage: loadAndHashImage,
- now: defaultNow,
- imgMatcherFactory: &imgmatching.MatcherFactoryImpl{},
+ workDir: workDir,
+ auth: authOpt,
+ loadAndHashImage: loadAndHashImage,
+ now: defaultNow,
resultState: newResultState(nil, &config),
}
@@ -232,11 +227,10 @@
return nil, skerr.Fmt("No 'workDir' provided to LoadCloudClient")
}
ret := CloudClient{
- workDir: workDir,
- auth: authOpt,
- loadAndHashImage: loadAndHashImage,
- now: defaultNow,
- imgMatcherFactory: &imgmatching.MatcherFactoryImpl{},
+ workDir: workDir,
+ auth: authOpt,
+ loadAndHashImage: loadAndHashImage,
+ now: defaultNow,
}
var err error
ret.resultState, err = loadStateFromJSON(ret.getResultStatePath())
@@ -451,7 +445,7 @@
// Extract the specified image matching algorithm from the optionalKeys (defaulting to exact
// matching if none is specified) and obtain an instance of the imgmatching.Matcher if the
// algorithm requires one (i.e. all but exact matching).
- algorithmName, matcher, err := c.imgMatcherFactory.Make(optionalKeys)
+ algorithmName, matcher, err := imgmatching.MakeMatcher(optionalKeys)
if err != nil {
return false, skerr.Wrapf(err, "parsing image matching algorithm from optional keys")
}
diff --git a/gold-client/go/imgmatching/factory.go b/gold-client/go/imgmatching/factory.go
index 74236aa..c9ee871 100644
--- a/gold-client/go/imgmatching/factory.go
+++ b/gold-client/go/imgmatching/factory.go
@@ -11,24 +11,12 @@
"go.skia.org/infra/gold-client/go/imgmatching/sobel"
)
-// MatcherFactory represents a Matcher factory that builds a Matcher from a map of optional keys.
-// It is implemented by MatcherFactoryImpl.
+// MakeMatcher takes a map of optional keys and returns the specified image matching algorithm
+// name, and the corresponding Matcher instance (or nil if none is specified).
//
-// The purpose of this interface is to make testing easier.
-type MatcherFactory interface {
- // Make takes a map of optional keys and returns the specified image matching algorithm name, and
- // the corresponding Matcher instance (or nil if none is specified).
- //
- // It returns a non-nil error if the specified image matching algorithm is invalid, or if any
- // required parameters are not found, or if the parameter values are not valid.
- Make(optionalKeys map[string]string) (AlgorithmName, Matcher, error)
-}
-
-// MatcherFactoryImpl implements the MatcherFactory interface.
-type MatcherFactoryImpl struct{}
-
-// Make implements the MatcherFactory interface.
-func (m MatcherFactoryImpl) Make(optionalKeys map[string]string) (AlgorithmName, Matcher, error) {
+// It returns a non-nil error if the specified image matching algorithm is invalid, or if any
+// required parameters are not found, or if the parameter values are not valid.
+func MakeMatcher(optionalKeys map[string]string) (AlgorithmName, Matcher, error) {
algorithmNameStr, ok := optionalKeys[AlgorithmOptionalKey]
algorithmName := AlgorithmName(algorithmNameStr)
@@ -157,6 +145,3 @@
return intVal, nil
}
-
-// Make sure MatcherFactoryImpl fulfills the MatcherFactory interface.
-var _ MatcherFactory = (*MatcherFactoryImpl)(nil)
diff --git a/gold-client/go/imgmatching/factory_test.go b/gold-client/go/imgmatching/factory_test.go
index 1ede82f..ad4178c 100644
--- a/gold-client/go/imgmatching/factory_test.go
+++ b/gold-client/go/imgmatching/factory_test.go
@@ -11,11 +11,10 @@
"go.skia.org/infra/gold-client/go/imgmatching/sobel"
)
-func TestMatcherFactoryImpl_Make_UnknownAlgorithm_ReturnsError(t *testing.T) {
+func TestMakeMatcher_UnknownAlgorithm_ReturnsError(t *testing.T) {
unittest.SmallTest(t)
- f := MatcherFactoryImpl{}
- _, _, err := f.Make(map[string]string{
+ _, _, err := MakeMatcher(map[string]string{
AlgorithmOptionalKey: "FakeAlgorithm",
})
@@ -23,22 +22,20 @@
assert.Contains(t, err.Error(), `unrecognized image matching algorithm: "FakeAlgorithm"`)
}
-func TestMatcherFactoryImpl_Make_NoAlgorithmSpecified_ReturnsExactMatching(t *testing.T) {
+func TestMakeMatcher_NoAlgorithmSpecified_ReturnsExactMatching(t *testing.T) {
unittest.SmallTest(t)
- f := MatcherFactoryImpl{}
- algorithmName, matcher, err := f.Make(map[string]string{})
+ algorithmName, matcher, err := MakeMatcher(map[string]string{})
assert.NoError(t, err)
assert.Equal(t, ExactMatching, algorithmName)
assert.Nil(t, matcher)
}
-func TestMatcherFactoryImpl_Make_ExactMatchingExplicitlySpecified_ReturnsExactMatching(t *testing.T) {
+func TestMakeMatcher_ExactMatchingExplicitlySpecified_ReturnsExactMatching(t *testing.T) {
unittest.SmallTest(t)
- f := MatcherFactoryImpl{}
- algorithmName, matcher, err := f.Make(map[string]string{
+ algorithmName, matcher, err := MakeMatcher(map[string]string{
AlgorithmOptionalKey: string(ExactMatching),
})
@@ -50,8 +47,8 @@
// missing is a sentinel value used to represent missing parameter values.
const missing = "missing value"
-// fuzzyMatchingTestCase represents a test case for MatcherFactoryImpl#Make() where a
-// fuzzy.FuzzyMatcher is instantiated.
+// fuzzyMatchingTestCase represents a test case for MakeMatcher() where a fuzzy.FuzzyMatcher is
+// instantiated.
type fuzzyMatchingTestCase struct {
name string
maxDifferentPixels string
@@ -63,8 +60,8 @@
// commonMaxDifferentPixelsTestCases returns test cases for the FuzzyMatchingMaxDifferentPixels
// optional key.
//
-// These tests are shared between TestMatcherFactoryImpl_Make_FuzzyMatching and
-// TestMatcherFactoryImpl_Make_SobelFuzzyMatching.
+// These tests are shared between TestMakeMatcher_FuzzyMatching and
+// TestMakeMatcher_SobelFuzzyMatching.
func commonMaxDifferentPixelsTestCases() []fuzzyMatchingTestCase {
return []fuzzyMatchingTestCase{
{
@@ -127,8 +124,8 @@
// commonMaxDifferentPixelsTestCases returns test cases for the FuzzyMatchingPixelDeltaThreshold
// optional key.
//
-// These tests are shared between TestMatcherFactoryImpl_Make_FuzzyMatching and
-// TestMatcherFactoryImpl_Make_SobelFuzzyMatching.
+// These tests are shared between TestMakeMatcher_FuzzyMatching and
+// TestMakeMatcher_SobelFuzzyMatching.
func commonPixelDeltaThresholdTestCases() []fuzzyMatchingTestCase {
return []fuzzyMatchingTestCase{
{
@@ -194,7 +191,7 @@
}
}
-func TestMatcherFactoryImpl_Make_FuzzyMatching(t *testing.T) {
+func TestMakeMatcher_FuzzyMatching(t *testing.T) {
unittest.SmallTest(t)
tests := []fuzzyMatchingTestCase{
@@ -220,7 +217,7 @@
optionalKeys[string(FuzzyMatchingPixelDeltaThreshold)] = tc.pixelDeltaThreshold
}
- algorithmName, matcher, err := MatcherFactoryImpl{}.Make(optionalKeys)
+ algorithmName, matcher, err := MakeMatcher(optionalKeys)
if tc.error != "" {
assert.Error(t, err)
@@ -234,7 +231,7 @@
}
}
-func TestMatcherFactoryImpl_Make_SobelFuzzyMatching(t *testing.T) {
+func TestMakeMatcher_SobelFuzzyMatching(t *testing.T) {
unittest.SmallTest(t)
type sobelFuzzyMatchingTestCase struct {
@@ -381,7 +378,7 @@
optionalKeys[string(FuzzyMatchingPixelDeltaThreshold)] = tc.pixelDeltaThreshold
}
- algorithmName, matcher, err := MatcherFactoryImpl{}.Make(optionalKeys)
+ algorithmName, matcher, err := MakeMatcher(optionalKeys)
if tc.error != "" {
assert.Error(t, err)
