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

brie: add integration test for br #18797

Merged
merged 7 commits into from
Jul 28, 2020

Conversation

3pointer
Copy link
Contributor

@3pointer 3pointer commented Jul 27, 2020

What problem does this PR solve?

Problem Summary: add BR integration for TiDB

What is changed and how it works?

What's Changed:

  1. BR only work on tikv store. so add a test in session_test which has tikv store.
  2. Since common: adjust config in task RunBackup/RunRestore br#443 handle all BR related configurations internally, we can remove useless setting config code

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Integration test

Release note

  • No release note

@3pointer 3pointer force-pushed the add_integration_test_for_br branch from 12eb04e to 24df6cf Compare July 27, 2020 09:25
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #18797 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18797   +/-   ##
===========================================
  Coverage   79.2275%   79.2275%           
===========================================
  Files           545        545           
  Lines        147840     147840           
===========================================
  Hits         117130     117130           
  Misses        21305      21305           
  Partials       9405       9405           

@3pointer 3pointer force-pushed the add_integration_test_for_br branch from 36282bb to 5fa4aae Compare July 27, 2020 09:56
@3pointer 3pointer requested a review from a team as a code owner July 27, 2020 09:56
@3pointer 3pointer requested review from qw4990 and removed request for a team July 27, 2020 09:56
@github-actions github-actions bot added the sig/execution SIG execution label Jul 27, 2020
@3pointer
Copy link
Contributor Author

/run-check_dev_2

@3pointer 3pointer force-pushed the add_integration_test_for_br branch from 02d31cb to 92e6461 Compare July 27, 2020 11:15
@3pointer
Copy link
Contributor Author

/run-check_dev_2


tk.MustQuery("select count(*) from t1").Check(testkit.Rows("3"))

os.RemoveAll("/tmp/bk1")
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using API like os.TempDir and path.Join to make the code more general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea

@3pointer 3pointer force-pushed the add_integration_test_for_br branch from 2bf9ad1 to 53e75d3 Compare July 28, 2020 03:54
@3pointer
Copy link
Contributor Author

/run-check_dev_2

@tiancaiamao
Copy link
Contributor

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 28, 2020
@glorv
Copy link
Contributor

glorv commented Jul 28, 2020

lgtm

@ti-srebot
Copy link
Contributor

@glorv,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@overvenus
Copy link
Member

LGTM

@ti-srebot
Copy link
Contributor

@overvenus,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@ti-srebot
Copy link
Contributor

@jebter,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@jebter
Copy link

jebter commented Jul 28, 2020

/merge

@ti-srebot
Copy link
Contributor

@jebter Oops! auto merge is restricted to Committers of the SIG.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

@zz-jason Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 28, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 28, 2020
@zz-jason
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 28, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 18668

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 5973d14 into pingcap:master Jul 28, 2020
3pointer added a commit to 3pointer/tidb that referenced this pull request Jul 28, 2020
ti-srebot pushed a commit that referenced this pull request Jul 29, 2020
3pointer added a commit to 3pointer/tidb that referenced this pull request Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants