*: replace defer clean with t.Cleanup #36722

xhebox · 2022-07-29T10:06:04Z

if there is no close, then you will never forget to close.

Signed-off-by: xhe [email protected]

What problem does this PR solve?

Issue Number: close #36721

Problem Summary: As title. Also includes a patch for bazel patches.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2022-07-29T10:06:05Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hawkingrei
tangenta

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

tangenta · 2022-07-29T10:27:49Z

@xhebox Please fix the linter.

[2022-07-29T10:23:22.718Z] br/pkg/backup/client_test.go:79:6: cannot initialize 1 variables with 2 values (typecheck)

[2022-07-29T10:23:22.718Z] 	s :=createBackupSuite(t)

[2022-07-29T10:23:22.718Z] 	    ^

[2022-07-29T10:23:22.718Z] br/pkg/backup/client_test.go:256:6: cannot initialize 1 variables with 2 values (typecheck)

[2022-07-29T10:23:22.718Z] 	s :=createBackupSuite(t)

[2022-07-29T10:23:22.718Z] 	    ^

[2022-07-29T10:23:22.718Z] br/pkg/backup/client_test.go:308:6: cannot initialize 1 variables with 2 values (typecheck)

[2022-07-29T10:23:22.718Z] 	s :=createBackupSuite(t)

[2022-07-29T10:23:22.718Z] 	    ^

[2022-07-29T10:23:22.718Z] dumpling/export/writer_test.go:333:62: missing ',' in parameter list (typecheck)

[2022-07-29T10:23:22.718Z] func createTestWriter(conf *Config, t *testing.T) (w *Writer func()) {

[2022-07-29T10:23:22.718Z]                                                              ^

[2022-07-29T10:23:22.718Z] store/gcworker/gc_worker_test.go:1978: File is not `goimports`-ed (goimports)

[2022-07-29T10:23:22.718Z] func bootstrap(t testing.TB, store kv.Storage, lease time.Duration) (*domain.Domain) {

[2022-07-29T10:23:22.718Z] executor/chunk_size_control_test.go:132: File is not `goimports`-ed (goimports)

[2022-07-29T10:23:22.718Z] 	kit :=createChunkSizeControlKit(t, "create table t (a int, index idx_a(a))")

hawkingrei · 2022-07-30T10:03:26Z

@xhebox

I find some code look like this:

tidb/executor/analyzetest/analyze_test.go

Lines 586 to 590 in cb211e0

    
           func TestSnapshotAnalyzeAndMaxTSAnalyze(t *testing.T) { 
        
           	for _, analyzeSnapshot := range []bool{true, false} { 
        
           		store, clean := testkit.CreateMockStore(t) 
        
           		defer clean() 
        
           		tk := testkit.NewTestKit(t, store)

if we are without the hand way to clean. we will meet some problems.

xhebox · 2022-08-01T03:05:36Z

@xhebox

I find some code look like this:

tidb/executor/analyzetest/analyze_test.go

Lines 586 to 590 in cb211e0

func TestSnapshotAnalyzeAndMaxTSAnalyze(t *testing.T) {

for _, analyzeSnapshot := range []bool{true, false} {

store, clean := testkit.CreateMockStore(t)

defer clean()

tk := testkit.NewTestKit(t, store)

if we are without the hand way to clean. we will meet some problems.

Could use subtests to handle it, tell me if there are other similar cases. Also, it will cause problem anyway, defer in loop does not make sense.

xhebox · 2022-08-01T04:00:15Z

@xhebox
I find some code look like this:

tidb/executor/analyzetest/analyze_test.go

Lines 586 to 590 in cb211e0

func TestSnapshotAnalyzeAndMaxTSAnalyze(t *testing.T) {

for _, analyzeSnapshot := range []bool{true, false} {

store, clean := testkit.CreateMockStore(t)

defer clean()

tk := testkit.NewTestKit(t, store)

if we are without the hand way to clean. we will meet some problems.

Could use subtests to handle it, tell me if there are other similar cases. Also, it will cause problem anyway, defer in loop does not make sense.

Fixed all cases in tests by revive linter, going to merge it.

/merge

ti-chi-bot · 2022-08-01T04:00:18Z

This pull request has been accepted and is ready to merge.

Commit hash: 5f80b734bd974d008d0c724dc97abd861f057b50

xhebox · 2022-08-01T04:24:34Z

/run-unit-test

xhebox · 2022-08-01T04:27:27Z

/merge

ti-chi-bot · 2022-08-01T04:27:30Z

This pull request has been accepted and is ready to merge.

Commit hash: d9ee5f6cb4545519fbc6a503fbf2e821596886fd

xhebox · 2022-08-01T04:52:53Z

/merge

xhebox · 2022-08-01T05:19:51Z

/run-unit-test

xhebox · 2022-08-01T06:56:11Z

/merge

hawkingrei · 2022-08-01T07:35:15Z

/run-all-tests

if there is no close, then you will never forget to close. Signed-off-by: xhe <[email protected]>

Signed-off-by: xhe <[email protected]>

hawkingrei · 2022-08-01T08:29:11Z

/merge

ti-chi-bot · 2022-08-01T08:29:15Z

This pull request has been accepted and is ready to merge.

Commit hash: 3ab19b7

hawkingrei · 2022-08-01T08:32:35Z

/run-build

ti-chi-bot · 2022-08-01T08:40:17Z

@xhebox: 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.

Signed-off-by: xhe <[email protected]>

xhebox · 2022-08-01T09:02:14Z

Every merge may damage this PR...

/merge

ti-chi-bot · 2022-08-01T09:02:19Z

This pull request has been accepted and is ready to merge.

Commit hash: e05802f

xhebox · 2022-08-01T09:17:53Z

/merge

close pingcap#36721

* upstream/master: ddl/schematracker: fix SetDDL will cause data race (pingcap#36768) planner: check virtual column for tiflash (pingcap#36771) *: replace defer clean with t.Cleanup (pingcap#36722) ddl: fix inaccurate row_count for `admin show ddl jobs` (pingcap#36716) config, sysvar: add config `instance.enable_ddl` and sysvar `tidb_enable_ddl` (pingcap#35425) *: enable part revive for all code (pingcap#36703)

xhebox requested review from hawkingrei and tangenta July 29, 2022 10:06

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2022

hawkingrei approved these changes Jul 29, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 29, 2022

tangenta approved these changes Jul 29, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 29, 2022

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2022

xhebox force-pushed the cleanup branch from 11bc082 to b534eb5 Compare August 1, 2022 03:04

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2022

xhebox requested review from tangenta and hawkingrei August 1, 2022 03:05

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

hawkingrei approved these changes Aug 1, 2022

View reviewed changes

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

xhebox added 8 commits August 1, 2022 15:57

*: replace defer clean with t.Cleanup

2863c1f

if there is no close, then you will never forget to close. Signed-off-by: xhe <[email protected]>

address comments

48d96bf

Signed-off-by: xhe <[email protected]>

*: fix CI

de0b5ad

Signed-off-by: xhe <[email protected]>

*: fix defer in loop

545f1f6

Signed-off-by: xhe <[email protected]>

*: fix CI

fa681b8

Signed-off-by: xhe <[email protected]>

*: fix CI panic

2566ad7

Signed-off-by: xhe <[email protected]>

*: fix CI

a9e7145

Signed-off-by: xhe <[email protected]>

*: fix last panic

be17c7b

Signed-off-by: xhe <[email protected]>

xhebox force-pushed the cleanup branch from 68e0a39 to be17c7b Compare August 1, 2022 07:57

*: fix merge

3ab19b7

Signed-off-by: xhe <[email protected]>

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

Merge branch 'master' into cleanup

5cc9de3

*: fix CI

e05802f

Signed-off-by: xhe <[email protected]>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 1, 2022

ti-chi-bot merged commit 9a16a8d into pingcap:master Aug 1, 2022

xhebox deleted the cleanup branch August 1, 2022 09:36

morgo pushed a commit to morgo/tidb that referenced this pull request Aug 1, 2022

*: replace defer clean with t.Cleanup (pingcap#36722)

8cef199

close pingcap#36721

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: replace defer clean with t.Cleanup #36722

*: replace defer clean with t.Cleanup #36722

xhebox commented Jul 29, 2022

ti-chi-bot commented Jul 29, 2022 •

edited

Loading

tangenta commented Jul 29, 2022

hawkingrei commented Jul 30, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

hawkingrei commented Aug 1, 2022

hawkingrei commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

hawkingrei commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

*: replace defer clean with t.Cleanup #36722

*: replace defer clean with t.Cleanup #36722

Conversation

xhebox commented Jul 29, 2022

What problem does this PR solve?

Check List

Release note

ti-chi-bot commented Jul 29, 2022 • edited Loading

tangenta commented Jul 29, 2022

hawkingrei commented Jul 30, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

xhebox commented Aug 1, 2022

hawkingrei commented Aug 1, 2022

hawkingrei commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

hawkingrei commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

ti-chi-bot commented Aug 1, 2022

xhebox commented Aug 1, 2022

ti-chi-bot commented Jul 29, 2022 •

edited

Loading