tester Service bug fixes (mostly around panicked test handling) (go…

…adesign#359)

* Make synch panic handling work, get name of panicked test via reflection, filter then not filtered call bug fix

* build(deps): bump golang.org/x/tools from 0.16.1 to 0.17.0 (goadesign#357)

Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.16.1 to 0.17.0.
- [Release notes](https://github.com/golang/tools/releases)
- [Commits](golang/tools@v0.16.1...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump goa.design/goa/v3 from 3.14.4 to 3.14.5 (goadesign#358)

Bumps goa.design/goa/v3 from 3.14.4 to 3.14.5.

---
updated-dependencies:
- dependency-name: goa.design/goa/v3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* simplify how `TestAll` works

* further simplify names where test is run to have single source of truth (the name retreived from reflection)

* use regex instead of glob so that the matched string can be returned

* use regex instead of glob for Exclude filter too

* remove glob library

* Update README with `*` wildcard info

* review fix

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

example/weather/go.mod

@@ -3,7 +3,6 @@ module goa.design/clue/example/weather
 go 1.21
 
 require (
-	github.com/gobwas/glob v0.2.3
 	github.com/stretchr/testify v1.8.4
 	go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.46.1
 	go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace v0.46.1

example/weather/go.sum

@@ -28,8 +28,6 @@ github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
 github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
 github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
 github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
-github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y=
-github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8=
 github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk=
 github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
 github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY=

example/weather/services/tester/README.md

@@ -54,6 +54,10 @@ exclude).
 `Include` and `Exclude` are mutually exclusive and cannot be used together. If that is done then
 the `TestAll` method will return a `400 Bad Request` error.
 
+`Include` and `Exclude` can accept `*`` wildcards to match multiple tests. For example, if you
+wanted to run all tests that start with `TestForecaster` you could pass `TestForecaster*` to the
+`Include` field.
+
 ### TestForecaster & TestLocator methods
 
 These methods run all tests defined for those services as found in `services/tester/func_map.go`.

example/weather/services/tester/forecaster_integration.go

@@ -12,7 +12,7 @@ import (
 func (svc *Service) TestForecasterValidLatLong(ctx context.Context, tc *TestCollection) {
 	results := []*gentester.TestResult{}
 	start := time.Now()
-	name := "Forecaster.TestValidLatLong"
+	name := "TestForecasterValidLatLong"
 	passed := true
 	testRes := gentester.TestResult{
 		Name:   name,
@@ -62,7 +62,7 @@ func (svc *Service) TestForecasterValidLatLong(ctx context.Context, tc *TestColl
 func (svc *Service) TestForecasterInvalidLat(ctx context.Context, tc *TestCollection) {
 	results := []*gentester.TestResult{}
 	start := time.Now()
-	name := "Forecaster.InvalidLat"
+	name := "TestForecasterInvalidLat"
 	passed := true
 	testRes := gentester.TestResult{
 		Name:   name,
@@ -98,7 +98,7 @@ func (svc *Service) TestForecasterInvalidLat(ctx context.Context, tc *TestCollec
 func (svc *Service) TestForecasterInvalidLong(ctx context.Context, tc *TestCollection) {
 	results := []*gentester.TestResult{}
 	start := time.Now()
-	name := "Forecaster.InvalidLong"
+	name := "TestForecasterInvalidLong"
 	passed := true
 	testRes := gentester.TestResult{
 		Name:   name,

example/weather/services/tester/locator_integration.go

@@ -11,7 +11,7 @@ import (
 func (svc *Service) TestLocatorValidIP(ctx context.Context, tc *TestCollection) {
 	results := []*gentester.TestResult{}
 	start := time.Now()
-	name := "Locator.TestValidIP"
+	name := "TestLocatorValidIP"
 	passed := true
 	testRes := gentester.TestResult{
 		Name:   name,
@@ -40,7 +40,7 @@ func (svc *Service) TestLocatorValidIP(ctx context.Context, tc *TestCollection)
 func (svc *Service) TestLocatorInvalidIP(ctx context.Context, tc *TestCollection) {
 	results := []*gentester.TestResult{}
 	start := time.Now()
-	name := "Locator.TestInvalidIP"
+	name := "TestLocatorInvalidIP"
 	passed := true
 	testRes := gentester.TestResult{
 		Name:   name,

example/weather/services/tester/run_tests.go

@@ -7,12 +7,12 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"regexp"
 	"runtime/debug"
 	"strings"
 	"sync"
 	"time"
 
-	"github.com/gobwas/glob"
 	"goa.design/clue/log"
 
 	gentester "goa.design/clue/example/weather/services/tester/gen/tester"
@@ -26,78 +26,121 @@ func endTest(tr *gentester.TestResult, start time.Time, tc *TestCollection, resu
 	tc.AppendTestResult(results...)
 }
 
-func getStackTrace(wg *sync.WaitGroup, m *sync.Mutex) string {
+func getStackTrace(wg *sync.WaitGroup, m *sync.Mutex) (string, error) {
 	m.Lock()
 	defer wg.Done()
 	defer m.Unlock()
 	// keep backup of the real stderr
 	old := os.Stderr
 	f, w, _ := os.Pipe()
 	os.Stderr = w
-	defer w.Close()
 
 	debug.PrintStack()
+	w.Close()
 
-	outC := make(chan string)
+	outC := make(chan string, 1)
+	outErr := make(chan error, 1)
 	go func() {
 		var buf bytes.Buffer
-		io.Copy(&buf, f) // nolint: errcheck
+		_, err := io.Copy(&buf, f)
+		outErr <- err
 		outC <- buf.String()
 	}()
 
 	// restoring the real stderr
 	os.Stderr = old
-	out := <-outC
 
-	return out
+	if err := <-outErr; err != nil {
+		return "", err
+	}
+	out := <-outC
+	return out, nil
 }
 
 // recovers from a panicked test. This is used to ensure that the test
 // suite does not crash if a test panics.
-func recoverFromTestPanic(ctx context.Context, testName string, testCollection *TestCollection) {
+func recoverFromTestPanic(ctx context.Context, testNameFunc func() string, testCollection *TestCollection) {
 	if r := recover(); r != nil {
-		msg := fmt.Sprintf("[Panic Test]: %v", testName)
+		msg := fmt.Sprintf("[Panic Test]: %v", testNameFunc())
 		err := errors.New(msg)
 		log.Errorf(ctx, err, fmt.Sprintf("%v", r))
 		var m sync.Mutex
 		var wg sync.WaitGroup
 		wg.Add(1)
-		trace := getStackTrace(&wg, &m)
+		trace, err := getStackTrace(&wg, &m)
 		wg.Wait()
-		err = fmt.Errorf("%v : %v", r, trace)
-		// log the error and add the test result to the test collection
-		_ = logError(ctx, err)
-		resultMsg := fmt.Sprintf("%v | %v", msg, r)
+		var resultMsg string
+		if err != nil {
+			err = fmt.Errorf("error getting stack trace for panicked test: %v", err)
+			resultMsg = err.Error()
+		} else {
+			err = fmt.Errorf("%v : %v", r, trace)
+			// log the error and add the test result to the test collection
+			_ = logError(ctx, err)
+			resultMsg = fmt.Sprintf("%v | %v", msg, r)
+		}
 		testCollection.AppendTestResult(&gentester.TestResult{
-			Name:     testName,
+			Name:     testNameFunc(),
 			Passed:   false,
 			Error:    &resultMsg,
 			Duration: -1,
 		})
 	}
 }
 
-// Filters a testMap based on a test name that is a glob string
-// using standard wildcards https://tldp.org/LDP/GNU-Linux-Tools-Summary/html/x11655.htm
-func matchTestFilter(ctx context.Context, test string, testMap map[string]func(context.Context, *TestCollection)) ([]func(context.Context, *TestCollection), error) {
-	match := false
-	var testMatches []func(context.Context, *TestCollection)
-	var g glob.Glob
-	g, err := glob.Compile(test)
+// Converts a wildcard string using * to a regular expression string
+func wildCardToRegexp(pattern string) string {
+	components := strings.Split(pattern, "*")
+	if len(components) == 1 {
+		// if len is 1, there are no *'s, return exact match pattern
+		return "^" + pattern + "$"
+	}
+	var result strings.Builder
+	for i, literal := range components {
+
+		// Replace * with .*
+		if i > 0 {
+			result.WriteString(".*")
+		}
+
+		// Quote any regular expression meta characters in the
+		// literal text.
+		result.WriteString(regexp.QuoteMeta(literal))
+	}
+	return "^" + result.String() + "$"
+}
+
+// wraps wildCardToRegexp and returns a bool indicating whether the value
+// matches the pattern, the string matched, and an error if one occurred
+func match(pattern string, value string) (bool, string, error) {
+	r, err := regexp.Compile(wildCardToRegexp(pattern))
 	if err != nil {
-		_ = logError(ctx, err)
-		err = fmt.Errorf("wildcard glob [%s] did not compile: %v", test, err)
-		return testMatches, err
+		return false, "", err
+	}
+	matches := r.FindStringSubmatch(value)
+	if len(matches) > 0 {
+		return true, matches[0], nil
+	} else {
+		return false, "", nil
 	}
+}
+
+// Filters a testMap based on a test name that is a glob string using only
+// * wildcards
+func matchTestFilterRegex(ctx context.Context, test string, testMap map[string]func(context.Context, *TestCollection)) (map[string]func(context.Context, *TestCollection), error) {
+	retval := make(map[string]func(context.Context, *TestCollection))
 	i := 0
 	for testName := range testMap {
-		match = g.Match(testName)
-		if match {
-			testMatches = append(testMatches, testMap[testName])
+		_, matchString, err := match(test, testName)
+		if err != nil {
+			return nil, err
+		}
+		if matchString != "" {
+			retval[matchString] = testMap[testName]
 		}
 		i++
 	}
-	return testMatches, nil
+	return retval, nil
 }
 
 // Runs the tests from the testmap and handles filtering/exclusion of tests
@@ -121,13 +164,13 @@ func (svc *Service) runTests(ctx context.Context, p *gentester.TesterPayload, te
 				if testFunc, ok := testMap[test]; ok {
 					testsToRun[test] = testFunc
 				} else { // Test didn't match exactly, so we're gonna try for a wildcard match
-					testFuncs, err := matchTestFilter(ctx, test, testMap)
+					testFuncs, err := matchTestFilterRegex(ctx, test, testMap)
 					if err != nil {
 						return nil, gentester.MakeWildcardCompileError(err)
 					}
 					if len(testFuncs) > 0 {
-						for i, testFunc := range testFuncs {
-							testsToRun[fmt.Sprintf("%s_%d", test, i)] = testFunc
+						for testName, testFunc := range testFuncs {
+							testsToRun[testName] = testFunc
 						}
 					} else { // No wildcard match either
 						err := fmt.Errorf("test [%v] not found in test map", test)
@@ -139,14 +182,11 @@ func (svc *Service) runTests(ctx context.Context, p *gentester.TesterPayload, te
 			for testName, test := range testMap {
 				wildcardMatch := false
 				for _, excludeTest := range p.Exclude {
-					var g glob.Glob
-					g, err := glob.Compile(excludeTest)
+					wildcardMatchThisTest, _, err := match(excludeTest, testName)
 					if err != nil {
-						_ = logError(ctx, err)
-						err = fmt.Errorf("wildcard glob [%s] did not compile: %v", excludeTest, err)
 						return nil, gentester.MakeWildcardCompileError(err)
 					}
-					wildcardMatch = wildcardMatch || g.Match(testName)
+					wildcardMatch = wildcardMatch || wildcardMatchThisTest
 				}
 				if !wildcardMatch {
 					testsToRun[testName] = test
@@ -162,22 +202,48 @@ func (svc *Service) runTests(ctx context.Context, p *gentester.TesterPayload, te
 
 	// Run the tests that need to be run and add the results to the testCollection.Results array
 	if runSynchronously {
+		// RunSynchronously is used for test collections that need to be run one after in order
+		// to avoid single resource contention between tests if they are run in parallel. An
+		// example of this is tests that rely on the same cloud resource, such as a spreadsheet,
+		// as part of their test functionality.
+		//
+		// testName is passed to recoverFromTestPanic as a function so that, via a closure, its
+		// name can be set before the test is run but after the defer of recoverFromTestPanic is
+		// declared. This is done because the test name is not accessible from within the test
+		// function itself where it is set.
 		log.Infof(ctx, "RUNNING TESTS SYNCHRONOUSLY")
-		for n, test := range testsToRun {
-			log.Infof(ctx, "RUNNING TEST [%v]", n)
-			test(ctx, testCollection)
-		}
+		wg := sync.WaitGroup{}
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			testName := ""
+			testNameFunc := func() string {
+				return testName
+			}
+			defer recoverFromTestPanic(ctx, testNameFunc, testCollection)
+			for testNameRunning, test := range testsToRun {
+				testName = testNameRunning
+				log.Infof(ctx, "RUNNING TEST [%v]", testName)
+				test(ctx, testCollection)
+			}
+		}()
+		wg.Wait()
 	} else {
+		// if not run synchronously, run the tests in parallel and assumed not to have resource
+		// contention
 		log.Infof(ctx, "RUNNING TESTS IN PARALLEL")
 		wg := sync.WaitGroup{}
-		for n, test := range testsToRun {
+		for name, test := range testsToRun {
 			wg.Add(1)
-			go func(f func(context.Context, *TestCollection), testName string) {
+			go func(f func(context.Context, *TestCollection), testNameRunning string) {
 				defer wg.Done()
-				defer recoverFromTestPanic(ctx, testName, testCollection)
-				log.Infof(ctx, "RUNNING TEST [%v]", testName)
+				testNameFunc := func() string {
+					return testNameRunning
+				}
+				defer recoverFromTestPanic(ctx, testNameFunc, testCollection)
+				log.Infof(ctx, "RUNNING TEST [%v]", testNameRunning)
 				f(ctx, testCollection)
-			}(test, n)
+			}(test, name)
 		}
 		wg.Wait()
 	}

example/weather/services/tester/test_methods.go

@@ -18,18 +18,25 @@ func (t *TestCollection) AppendTestResult(tr ...*gentester.TestResult) {
 	t.Results = append(t.Results, tr...)
 }
 
-var filteringPayload = &gentester.TesterPayload{}
-
 // Runs all test collections EXCEPT smoke tests (those are in their own collections as well)
 func (svc *Service) TestAll(ctx context.Context, p *gentester.TesterPayload) (res *gentester.TestResults, err error) {
 	retval := gentester.TestResults{}
-	filteringPayload = p
-	forecasterResults, err := svc.TestForecaster(ctx)
+
+	// Forecaster tests
+	forecasterCollection := TestCollection{
+		Name: "Forecaster Tests",
+	}
+	forecasterResults, err := svc.runTests(ctx, p, &forecasterCollection, svc.forecasterTestMap, false)
 	if err != nil {
 		_ = logError(ctx, err)
 		return nil, err
 	}
-	locatorResults, err := svc.TestLocator(ctx)
+
+	// Locator tests
+	locatorCollection := TestCollection{
+		Name: "Locator Tests",
+	}
+	locatorResults, err := svc.runTests(ctx, p, &locatorCollection, svc.locatorTestMap, true)
 	if err != nil {
 		_ = logError(ctx, err)
 		return nil, err
@@ -55,23 +62,23 @@ func (svc *Service) TestSmoke(ctx context.Context) (res *gentester.TestResults,
 	smokeCollection := TestCollection{
 		Name: "Smoke Tests",
 	}
-	return svc.runTests(ctx, filteringPayload, &smokeCollection, svc.smokeTestMap, false)
+	return svc.runTests(ctx, nil, &smokeCollection, svc.smokeTestMap, false)
 }
 
-// Runs the ACL Service tests as a collection in parallel
+// Runs the Forecaster Service tests as a collection in parallel
 func (svc *Service) TestForecaster(ctx context.Context) (res *gentester.TestResults, err error) {
-	// ACL tests
-	aclCollection := TestCollection{
+	// Forecaster tests
+	forecasterCollection := TestCollection{
 		Name: "Forecaster Tests",
 	}
-	return svc.runTests(ctx, filteringPayload, &aclCollection, svc.forecasterTestMap, false)
+	return svc.runTests(ctx, nil, &forecasterCollection, svc.forecasterTestMap, false)
 }
 
-// Runs the Login Service tests as a collection synchronously
+// Runs the Locator Service tests as a collection synchronously
 func (svc *Service) TestLocator(ctx context.Context) (res *gentester.TestResults, err error) {
-	// Login tests
-	loginCollection := TestCollection{
+	// Locator tests
+	locatorCollection := TestCollection{
 		Name: "Locator Tests",
 	}
-	return svc.runTests(ctx, filteringPayload, &loginCollection, svc.locatorTestMap, true)
+	return svc.runTests(ctx, nil, &locatorCollection, svc.locatorTestMap, true)
 }