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

test(gRPC): remove redundant unit tests #1631

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

DMwangnima
Copy link
Contributor

@DMwangnima DMwangnima commented Dec 2, 2024

What type of PR is this?

test

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:

  1. Adjust KeepAlive unit tests to eliminate t.Parallel()
  2. Currently leakcheck is not used, skip the unit test to save about 4s of time
go test -race -count=1 ./...
ok      github.com/cloudwego/kitex/pkg/remote/trans/nphttp2/grpc/testutils/leakcheck    4.659s

zh(optional):

(Optional) Which issue(s) this PR fixes:

(optional) The PR that updates user documentation:

@DMwangnima DMwangnima requested review from a team as code owners December 2, 2024 02:42
@DMwangnima DMwangnima force-pushed the test/better_grpc_testing_prompt branch from a1c3ebb to 7540c58 Compare December 2, 2024 02:57
},
}
server, client := setUpWithOptions(t, 0, serverConfig, suspended, ConnectOptions{
// FIXME the original ut don't contain KeepaliveParams
Copy link
Contributor Author

Choose a reason for hiding this comment

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

应该是理解错了这个单元测试的用意,整个流程是由 server 来主动发送 Ping,client 不需要主动发,所以不需要配置 KeepAlive 参数

ppzqh
ppzqh previously approved these changes Dec 2, 2024

// grpcLogger is used to add test case information to assist in troubleshooting problems.
// it relies on testNameKey:testcase kv in ctx.
type grpcLogger struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

正常情况下,如果单测没问题的话,go test ./... 是会跳过输出的,看不懂这个的用意是什么。我看相关的单测也不是并发的,有什么问题?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

单测出了问题的话,打出来的 Log 就比较混乱了,不好排查问题,这是一个示例:
image

@DMwangnima DMwangnima force-pushed the test/better_grpc_testing_prompt branch from 37da49f to aba6a07 Compare December 3, 2024 03:47
@DMwangnima DMwangnima changed the title test(gRPC): add test case prompt and remove redundant unit tests test(gRPC): remove redundant unit tests Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants