`tester` Service bug fixes (mostly around panicked test handling) #359

jasondborneman · 2024-01-16T21:12:02Z

Rename tests to match test func names. This makes test names in the return match the func names for easier debugging.

run_tests.go

Fix getStackTrace. It had a defer w.Close() that was making it hang + it needed to check for error from io.Copy
change wildcard filtering to use a regex instead of a glob library. This allows for return of the matched string, which in turn makes it possible to always know the true name of a test func even when filtering by wildcards

test_methods.go

Rework how TestAll works. Instead of calling the other goa gRPC funcs (TestLocator, TestForecaster) call runTests directly and define the collections in TestAll. This makes it so I don't have a global filtering payload var, which was causing a bug when you make a filtered TestAll call then try to make an unfiltered call (filtering not available except for TestAll) to another func like TestSmoke.

…ion, filter then not filtered call bug fix

…#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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

) 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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

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

simplify how `TestAll` works

…th (the name retreived from reflection)

raphael

Left a few comments, the TL;DR is that It would be good to come up with a solution that does not involve reflection.

raphael · 2024-01-17T03:15:35Z

example/weather/services/tester/run_tests.go

 				log.Infof(ctx, "RUNNING TEST [%v]", testName)
 				f(ctx, testCollection)
-			}(test, n)


The previous version was much better :)

@raphael not sure I follow, I took the testName out because I am using getTestName and didn't need to pass in testName anymore to the goroutine. If I figure out how to get testName without reflection this may go back though.

raphael · 2024-01-17T03:16:52Z

example/weather/services/tester/run_tests.go

+			}
+			defer recoverFromTestPanic(ctx, testNameFunc, testCollection)
+			for _, test := range testsToRun {
+				testName = getTestName(test)


I am not sure I understand the need for getTestName. In general using reflection for a service at runtime isn't great (it's slow) and a bit of a code smell. At the very least if we were to use getTestName we could get rid of lines 218-221. But it would be much better to find a solution that does not require it. Is testsToRun not indexed by test name?

@raphael It is, but I was having trouble getting the true test name in the case of wildcard filters. I'll take some time to see if I can't rework this. If I fail to do that I may need to keep it though so that the true test name is returned, especially when there is a panic so that the exact test that is panicking can be identified

raphael · 2024-01-17T03:19:59Z

example/weather/services/tester/run_tests.go

-		io.Copy(&buf, f) // nolint: errcheck
-		outC <- buf.String()
+		_, err := io.Copy(&buf, f)
+		if err != nil {


I am not sure the if is required. If the error is nil then outErr <- err works fine and if there's an error then buf.String() should return "" (buffer is empty).

OK cool. I was being overly cautious. :) Will change

raphael · 2024-01-17T03:20:49Z

example/weather/services/tester/run_tests.go

-	return out
+	if err := <-outErr; err != nil {
+		return "", err
+	} else {


The else is not needed.

raphael · 2024-01-17T03:21:46Z

example/weather/services/tester/run_tests.go

+		if err != nil {
+			err = fmt.Errorf("error getting stack trace for panicked test: %v", err)
+			resultMsg := err.Error()
+			testCollection.AppendTestResult(&gentester.TestResult{


This could be taken out of the if (since the else does the same thing).

jasondborneman · 2024-01-17T14:17:53Z

@raphael I figured it out using a regex instead of a glob. This eliminates the need for reflection. Take a look and let me know what you think.

codecov-commenter · 2024-01-17T14:32:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b5e85f7) 93.24% compared to head (eccdf28) 93.24%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #359   +/-   ##
=======================================
  Coverage   93.24%   93.24%           
=======================================
  Files          32       32           
  Lines        1822     1822           
=======================================
  Hits         1699     1699           
  Misses        100      100           
  Partials       23       23

Flag	Coverage Δ
micro	`93.24% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

raphael · 2024-01-17T18:24:02Z

example/weather/services/tester/run_tests.go

 				defer wg.Done()
-				defer recoverFromTestPanic(ctx, testName, testCollection)
-				log.Infof(ctx, "RUNNING TEST [%v]", testName)
+				testName := ""


This could be simplified to:

testNameFunc := func() string { return testNameRunning }

No need for the testName variable.

@raphael fixed.

raphael

Looks good, left a small comment

jasondborneman and others added 7 commits January 16, 2024 15:49

Make synch panic handling work, get name of panicked test via reflect…

ce8adc4

…ion, filter then not filtered call bug fix

Merge pull request #16 from jasondborneman/bugfixes011624

8901f30

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

simplify how TestAll works

8a697d4

Merge pull request #17 from jasondborneman/bugfixes011624

ecf2768

simplify how `TestAll` works

further simplify names where test is run to have single source of tru…

c7e707a

…th (the name retreived from reflection)

raphael reviewed Jan 17, 2024

View reviewed changes

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

14685f4

use regex instead of glob for Exclude filter too

c47f65c

remove glob library

954c4d1

raphael reviewed Jan 17, 2024

View reviewed changes

jasondborneman added 2 commits January 17, 2024 13:32

Update README with * wildcard info

772eaf7

review fix

eccdf28

raphael merged commit b76a0f9 into goadesign:main Jan 19, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`tester` Service bug fixes (mostly around panicked test handling) #359

`tester` Service bug fixes (mostly around panicked test handling) #359

jasondborneman commented Jan 16, 2024 •

edited

Loading

raphael left a comment

raphael Jan 17, 2024

jasondborneman Jan 17, 2024

raphael Jan 17, 2024

jasondborneman Jan 17, 2024

raphael Jan 17, 2024

jasondborneman Jan 17, 2024

raphael Jan 17, 2024

raphael Jan 17, 2024

jasondborneman commented Jan 17, 2024 •

edited

Loading

codecov-commenter commented Jan 17, 2024 •

edited

Loading

raphael Jan 17, 2024

jasondborneman Jan 17, 2024

raphael left a comment

tester Service bug fixes (mostly around panicked test handling) #359

tester Service bug fixes (mostly around panicked test handling) #359

Conversation

jasondborneman commented Jan 16, 2024 • edited Loading

raphael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasondborneman commented Jan 17, 2024 • edited Loading

codecov-commenter commented Jan 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphael left a comment

Choose a reason for hiding this comment

`tester` Service bug fixes (mostly around panicked test handling) #359

`tester` Service bug fixes (mostly around panicked test handling) #359

jasondborneman commented Jan 16, 2024 •

edited

Loading

jasondborneman commented Jan 17, 2024 •

edited

Loading

codecov-commenter commented Jan 17, 2024 •

edited

Loading