Fix flake: Don't rely on variable allProblems in test (#5801)

* wip

* don't rely on variable  in test

pkg/skaffold/build/build_problems.go

@@ -41,15 +41,7 @@ var (
 	nilSuggestions = func(cfg interface{}) []*proto.Suggestion { return nil }
 	// for testing
 	getConfigForCurrentContext = config.GetConfigForCurrentKubectx
-)
-
-// re is a shortcut around regexp.MustCompile
-func re(s string) *regexp.Regexp {
-	return regexp.MustCompile(s)
-}
-
-func init() {
-	sErrors.AddPhaseProblems(constants.Build, []sErrors.Problem{
+	problems                   = []sErrors.Problem{
 		{
 			Regexp:  re(fmt.Sprintf(".*%s.* denied: .*", PushImageErr)),
 			ErrCode: proto.StatusCode_BUILD_PUSH_ACCESS_DENIED,
@@ -96,7 +88,16 @@ func init() {
 				}}
 			},
 		},
-	})
+	}
+)
+
+// re is a shortcut around regexp.MustCompile
+func re(s string) *regexp.Regexp {
+	return regexp.MustCompile(s)
+}
+
+func init() {
+	sErrors.AddPhaseProblems(constants.Build, problems)
 }
 
 func suggestBuildPushAccessDeniedAction(cfg interface{}) []*proto.Suggestion {

pkg/skaffold/build/build_problems_test.go

@@ -181,6 +181,11 @@ func TestBuildProblems(t *testing.T) {
 			t.Override(&getConfigForCurrentContext, func(string) (*config.ContextConfig, error) {
 				return &test.context, nil
 			})
+			t.Override(&sErrors.GetProblemCatalogCopy, func() sErrors.ProblemCatalog {
+				pc := sErrors.NewProblemCatalog()
+				pc.AddPhaseProblems(constants.Build, problems)
+				return pc
+			})
 			cfg := mockConfig{optRepo: test.optRepo}
 			actual := sErrors.ShowAIError(&cfg, test.err)
 			t.CheckDeepEqual(test.expected, actual.Error())

pkg/skaffold/deploy/deploy_problems.go

@@ -29,6 +29,19 @@ import (
 
 var (
 	clusterConnectionErr = regexp.MustCompile("(?i).*unable to connect.*: Get (.*)")
+	problems             = []sErrors.Problem{
+		{
+			Regexp:  clusterConnectionErr,
+			ErrCode: proto.StatusCode_DEPLOY_CLUSTER_CONNECTION_ERR,
+			Description: func(err error) string {
+				if match := clusterConnectionErr.FindStringSubmatch(err.Error()); len(match) >= 2 {
+					return fmt.Sprintf("Deploy Failed. Could not connect to cluster due to %s", match[1])
+				}
+				return "Deploy Failed. Could not connect to cluster."
+			},
+			Suggestion: suggestDeployFailedAction,
+		},
+	}
 )
 
 func suggestDeployFailedAction(cfg interface{}) []*proto.Suggestion {
@@ -48,17 +61,5 @@ func suggestDeployFailedAction(cfg interface{}) []*proto.Suggestion {
 }
 
 func init() {
-	sErrors.AddPhaseProblems(constants.Deploy, []sErrors.Problem{
-		{
-			Regexp:  clusterConnectionErr,
-			ErrCode: proto.StatusCode_DEPLOY_CLUSTER_CONNECTION_ERR,
-			Description: func(err error) string {
-				if match := clusterConnectionErr.FindStringSubmatch(err.Error()); len(match) >= 2 {
-					return fmt.Sprintf("Deploy Failed. Could not connect to cluster due to %s", match[1])
-				}
-				return "Deploy Failed. Could not connect to cluster."
-			},
-			Suggestion: suggestDeployFailedAction,
-		},
-	})
+	sErrors.AddPhaseProblems(constants.Deploy, problems)
 }

pkg/skaffold/deploy/deploy_problems_test.go

@@ -84,6 +84,11 @@ func TestSuggestDeployFailedAction(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.Override(&sErrors.GetProblemCatalogCopy, func() sErrors.ProblemCatalog {
+				pc := sErrors.NewProblemCatalog()
+				pc.AddPhaseProblems(constants.Deploy, problems)
+				return pc
+			})
 			cfg := mockConfig{kubeContext: test.context}
 			if test.isMinikube {
 				cfg.minikube = test.context

pkg/skaffold/errors/errors.go

@@ -82,9 +82,7 @@ func ShowAIError(cfg interface{}, err error) error {
 		return p.AIError(cfg, err)
 	}
 
-	allErrorsLock.RLock()
-	defer allErrorsLock.RUnlock()
-	for _, problems := range allErrors {
+	for _, problems := range GetProblemCatalogCopy().allErrors {
 		for _, p := range problems {
 			if p.Regexp.MatchString(err.Error()) {
 				instrumentation.SetErrorCode(p.ErrCode)
@@ -101,7 +99,7 @@ func getErrorCodeFromError(cfg interface{}, phase constants.Phase, err error) (p
 		return sErr.StatusCode(), sErr.Suggestions()
 	}
 
-	if problems, ok := allErrors[phase]; ok {
+	if problems, ok := GetProblemCatalogCopy().allErrors[phase]; ok {
 		for _, p := range problems {
 			if p.Regexp.MatchString(err.Error()) {
 				return p.ErrCode, p.Suggestion(cfg)

pkg/skaffold/errors/problem.go

@@ -27,8 +27,8 @@ import (
 )
 
 var (
-	allErrors     = map[constants.Phase][]Problem{}
-	allErrorsLock sync.RWMutex
+	problemCatalog      ProblemCatalog
+	addPhaseProblemLock sync.RWMutex
 )
 
 type descriptionFunc func(error) string
@@ -80,10 +80,10 @@ func isProblem(err error) (Problem, bool) {
 }
 
 func AddPhaseProblems(phase constants.Phase, problems []Problem) {
-	allErrorsLock.Lock()
-	if ps, ok := allErrors[phase]; ok {
-		problems = append(ps, problems...)
+	addPhaseProblemLock.Lock()
+	if problemCatalog.allErrors == nil {
+		problemCatalog = NewProblemCatalog()
 	}
-	allErrors[phase] = problems
-	allErrorsLock.Unlock()
+	problemCatalog.AddPhaseProblems(phase, problems)
+	addPhaseProblemLock.Unlock()
 }

pkg/skaffold/errors/problem_catalog.go

@@ -0,0 +1,50 @@
+/*
+Copyright 2021 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package errors
+
+import (
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
+)
+
+var (
+	// GetProblemCatalogCopy get a copies of the current problem catalog.
+	GetProblemCatalogCopy = getProblemCatalogCopy
+)
+
+type ProblemCatalog struct {
+	allErrors map[constants.Phase][]Problem
+}
+
+func (p ProblemCatalog) AddPhaseProblems(phase constants.Phase, problems []Problem) {
+	if ps, ok := p.allErrors[phase]; ok {
+		problems = append(ps, problems...)
+	}
+	p.allErrors[phase] = problems
+}
+
+func getProblemCatalogCopy() ProblemCatalog {
+	return ProblemCatalog{
+		allErrors: problemCatalog.allErrors,
+	}
+}
+
+func NewProblemCatalog() ProblemCatalog {
+	problemCatalog = ProblemCatalog{
+		allErrors: map[constants.Phase][]Problem{},
+	}
+	return problemCatalog
+}

pkg/skaffold/initializer/init_problems.go

@@ -24,13 +24,8 @@ import (
 	"github.com/GoogleContainerTools/skaffold/proto/v1"
 )
 
-// re is a shortcut around regexp.MustCompile
-func re(s string) *regexp.Regexp {
-	return regexp.MustCompile(s)
-}
-
-func init() {
-	sErrors.AddPhaseProblems(constants.Init, []sErrors.Problem{
+var (
+	problems = []sErrors.Problem{
 		{
 			Regexp:     re(".*creating tagger.*"),
 			ErrCode:    proto.StatusCode_INIT_CREATE_TAGGER_ERROR,
@@ -77,5 +72,14 @@ func init() {
 			ErrCode:    proto.StatusCode_INIT_CACHE_ERROR,
 			Suggestion: sErrors.ReportIssueSuggestion,
 		},
-	})
+	}
+)
+
+// re is a shortcut around regexp.MustCompile
+func re(s string) *regexp.Regexp {
+	return regexp.MustCompile(s)
+}
+
+func init() {
+	sErrors.AddPhaseProblems(constants.Init, problems)
 }

pkg/skaffold/initializer/init_problems_test.go

@@ -115,6 +115,11 @@ func TestInitProblems(t *testing.T) {
 	}
 	for _, test := range initTestCases {
 		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.Override(&sErrors.GetProblemCatalogCopy, func() sErrors.ProblemCatalog {
+				pc := sErrors.NewProblemCatalog()
+				pc.AddPhaseProblems(constants.Init, problems)
+				return pc
+			})
 			actual := sErrors.ShowAIError(nil, test.err)
 			t.CheckDeepEqual(test.expected, actual.Error())
 			actualAE := sErrors.ActionableErr(nil, constants.Init, test.err)