Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified tests to use tlogger. #3343

Merged
merged 15 commits into from
Feb 6, 2020
Merged

Modified tests to use tlogger. #3343

merged 15 commits into from
Feb 6, 2020

Conversation

GarrettGutierrez1
Copy link
Contributor

Modified most tests to use tlogger and leak checker from grpctest.

Updates #3006

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple tiny things that won't depend on your upcoming changes.

balancer/grpclb/grpclb_config_test.go Outdated Show resolved Hide resolved
balancer/grpclb/grpclb_config_test.go Outdated Show resolved Hide resolved
internal/grpctest/tlogger.go Outdated Show resolved Hide resolved
internal/grpctest/tlogger.go Show resolved Hide resolved
balancer/grpclb/grpclb_test.go Outdated Show resolved Hide resolved
@dfawley dfawley removed their assignment Feb 3, 2020
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good - just a couple minor things (reporting unmet errors and some minor restructuring).

internal/grpctest/tlogger.go Outdated Show resolved Hide resolved
logger.errors[re] += n
}

// ErrorsLeft checks if expected errors were not encountered.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make sure the unencountered expectations are reported by name. I think passing the testing.T here and calling t.Errorf() would be easiest; then you can remove the return value and call this EndTest or Cleanup or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function is changed to EndTest() and it reports the unmatched error regular expressions by name now.

internal/grpctest/tlogger.go Show resolved Hide resolved
internal/grpctest/tlogger.go Outdated Show resolved Hide resolved
internal/grpctest/tlogger.go Outdated Show resolved Hide resolved
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple small things in a test. Thanks!

TLogger.ExpectError("Expected error")
TLogger.ExpectError("Expected ln error")
TLogger.ExpectError("Expected formatted error")
TLogger.ExpectErrorN("Expected repeated error", 10)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10->numErrors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this.

internal/grpctest/tlogger.go Show resolved Hide resolved
splitFuncName := strings.Split(runtime.FuncForPC(reflect.ValueOf(testFunc).Pointer()).Name(), ".")
t.Run(splitFuncName[len(splitFuncName)-1], testFunc)
func (s) TestError(t *testing.T) {
numErrors := 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const numErrors = 10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@GarrettGutierrez1 GarrettGutierrez1 merged commit 132187f into grpc:master Feb 6, 2020
@GarrettGutierrez1 GarrettGutierrez1 deleted the use-tlogger-on-tests branch February 6, 2020 22:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants