Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

add dumpling unit test #1115

Merged
merged 12 commits into from
Oct 9, 2020
Merged

add dumpling unit test #1115

merged 12 commits into from
Oct 9, 2020

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Sep 28, 2020

What problem does this PR solve?

part of #998

What is changed and how it works?

add dumpling ut

Check List

Tests

  • Unit test

@GMHDBJD GMHDBJD added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes labels Sep 28, 2020
Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

rest LGTM

dumpling/dumpling.go Outdated Show resolved Hide resolved
dumpling/dumpling_test.go Outdated Show resolved Hide resolved
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Sep 30, 2020

/run-all-test

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Sep 30, 2020

/run-all-tests

1 similar comment
@csuzhangxc
Copy link
Member

/run-all-tests

Comment on lines 110 to 117
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
dumpling.Process(ctx, resultCh)
}()
cancel()
wg.Wait()
Copy link
Member

Choose a reason for hiding this comment

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

the extra goroutine can still be removed?

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Sep 30, 2020

/run-all-tests

Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

rest LGTM

c.Assert(len(resultCh), Equals, 1)
result := <-resultCh
c.Assert(result.IsCanceled, IsFalse)
c.Assert(len(result.Errors), Equals, 0, Commentf("errors: %v", result.Errors))
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to remove this Commentf since it's zero-length


func (d *testDumplingSuite) TestDumpling(c *C) {
dumpling := NewDumpling(d.cfg)
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about only use WithTimeout in third dumpling.Process, I'm afraid when CI runs slowly it will fail

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Oct 9, 2020
@lance6716
Copy link
Collaborator

LGTM

@lance6716 lance6716 added the needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 label Oct 9, 2020
@csuzhangxc csuzhangxc merged commit a247853 into pingcap:master Oct 9, 2020
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Oct 9, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1131

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Oct 9, 2020
csuzhangxc pushed a commit that referenced this pull request Oct 9, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: GMHDBJD <35025882+GMHDBJD@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT1 One reviewer already commented LGTM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants