-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
executor: migrate TestBatchInsertWithOnDuplicate to testify #26712
Conversation
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/cc @mmyj |
@tisonkun: GitHub didn't allow me to request PR reviews from the following users: mmyj. Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
executor/main_test.go
Outdated
goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"), | ||
goleak.IgnoreTopFunction("gopkg.in/natefinch/lumberjack%2ev2.(*Logger).millRun"), | ||
} | ||
goleak.VerifyTestMain(m, opts...) |
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.
why do we need to ignore these functions? how do we determine which function to ignore?
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.
see also https://internals.tidb.io/t/topic/280/2 They are already ignored by testleak
and when we migrate to goleak
, we repeat it.
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.
Hopefully we later fix all unreasonable leak, but we don't force it in this PR IMO.
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.
Because all the scope consists of other tests expect TestBatchInsertWithOnDuplicate
, and actually, TestBatchInsertWithOnDuplicate
doesn't suffer.
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.
Sure, thanks for the explanation.
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 602d8dc
|
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.
I guess the leak is caused by other cases.
If you run
go test TestBatchInsertWithOnDuplicate
There is no leak from our repo.
@tiancaiamao Yes it is not caused by |
Not a committer, and I will wait for the opionion from tiancaiamao. |
I'm testing this branch, using binary search to locate the leak case.
Maybe when it fails when including the TestT ... let me have a try |
|
This is the root cause @tisonkun |
@tiancaiamao thanks! I think we can comment this information on #26854 and fix them on the fly? Or you think it block this PR to merge? |
I've filed a PR to fix the leak I found #26881 IMO, add a whitelist of So what's the purpose of the test case restructing? we want the code to be maintainable, the test case to be more stable ... this time it's a good chance for us to improve, but if we add the bad cases into the whitelist, nothing improved. We can't achieve anything we really wanted ... |
Agree and thank you for the PR! |
@tiancaiamao actually I keep a same opinion with you as already posted on https://internals.tidb.io/t/topic/200/3?u=tison. My suggestion is unblocking this PR and later drop unreasonable leak ignores. You may think "later" be never and that's great you help on fixing it right now. |
Given #26881 merged, let me try to remove the whitelist. |
@tiancaiamao there are still leaks https://gist.github.com/tisonkun/01c6fa0778849b1ca330efb5f29e3e0b |
If you don't like whitelist, we can avoid using |
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tiancaiamao I push two commits to narrow goleak validation in the migrated functions. PTAL |
Ummm well goleak verify stack and we can hardly run it separatedly |
Signed-off-by: tison <wander4096@gmail.com>
session.SetSchemaLease(0) | ||
session.DisableStats4Test() | ||
d, err := session.BootstrapSession(store) |
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.
I have a concern this will make our CI take a lot of time than before.
In the past, one test suite create one store/domain.
If after the refactor, each test case create one store/domain, the operation is heavier than before... session.BootstrapSession
is not cheap.
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.
We can solve it later, through~
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.
OK. I'll think of reusing store/domain between tests. Maybe we come back to batch several tests in serial with testify suite.
/merge |
This pull request has been accepted and is ready to merge. Commit hash: f70c4ae
|
@tisonkun: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: close #26160
See also commits, I already separated into three commits.
Release note