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: add more test for pkg network #672
Conversation
Please fix CI |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #672 +/- ##
===========================================
+ Coverage 72.49% 73.92% +1.42%
===========================================
Files 96 96
Lines 9392 9392
===========================================
+ Hits 6809 6943 +134
+ Misses 2154 2009 -145
- Partials 429 440 +11 see 5 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
netpoll does not support tls, so there is no need to care about tls.config in netpoll. |
Yes, I'm not sure if we need to return |
I think it's ok to return this. |
Thanks for your report! Optimize PR is welcome~ |
Can I create an optimized PR after this PR is complete? because their tests may conflict. https://github.com/cloudwego/hertz/pull/672/files#diff-da9b81109f65e95b79b7f99cde6f9b44a23119bfe16028b3747b161b2e307bacR62 |
Sure~ |
Please check the failed UT in CI 🙏. Not sure whether it is related to this change or not. |
I tried to reproduce it locally or in docker, but all failed, and I didn't find relevant information in https://github.com/valyala/bytebufferpool. It is very frequently reproduced in the github action, and it always times out in the first goroutine. I tried to calculate the time it takes in the action by executing testPoolVariousSizes once first (excluding the impact of optimistic locks), and the result failed at 5.98s, indicating that the first func TestPoolVariousSizesConcurrent(t *testing.T) {
+ testPoolVariousSizes(t)
concurrency := 5
... I think it takes a lot of time for memory allocation in action even if sync.Pool is used (the machine provided by action is slower), maybe we should increase the timeout time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx~
What type of PR is this?
test
Check the PR title.
(Optional) Translate the PR title into Chinese.
添加
pkg/network
单测(Optional) More detail description for this PR(en: English/zh: Chinese).
en: Supplement unit test for
pkg/network
, coverage increased from 68.14% to 95.2%.I see that netpoll's DialTimeout doesn't check tlsConfig, I'm not sure if it's a bug
https://github.com/cloudwego/hertz/blob/develop/pkg/network/netpoll/dial.go#L48
zh(optional): 补充
pkg/network
的单元测试,覆盖率从68.14%提高到了95.2%.。我看到netpoll的DialTimeout并没有检查
tlsConfig
,我不确定是不是个错误。Which issue(s) this PR fixes:
related to #257