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

mock: enable the mock package only in unit tests #838

Merged
merged 2 commits into from
Mar 10, 2021

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Mar 10, 2021

What problem does this PR solve?

Close #837. Allows TiDB to depend on BR without pulling in Lightning's dependencies yet.

What is changed and how it works?

Make the mock package (currently depending on backend package which in turn pulls in pebble and the whole tree) require the build tag br_test. The tag is only active in unit tests.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
    • The mock package is no longer allowed outside of unit tests.

Side effects

Related changes

  • Need to cherry-pick to the release branch

Release Note

  • No release note

@overvenus
Copy link
Member

LGTM

@glorv
Copy link
Collaborator

glorv commented Mar 10, 2021

Do we need to update the project document since after this change, run go test command must always add the -tags br-test parameter.

@kennytm
Copy link
Collaborator Author

kennytm commented Mar 10, 2021

@glorv I thought we only support make test?

@glorv
Copy link
Collaborator

glorv commented Mar 10, 2021

It's convenient to run go test under a single or run command like go test gihub.com/pingcap/br/pkg/lightning.backend to avoid run all the tests during development.

Developers (like me?) for community may came across this issue if they forget to add the -tags br-test when run go test manually.

@AndreMouche
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

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

@AndreMouche
Copy link
Contributor

It's convenient to run go test under a single or run command like go test gihub.com/pingcap/br/pkg/lightning.backend to avoid run all the tests during development.

Developers (like me?) for community may came across this issue if they forget to add the -tags br-test when run go test manually.

I would like to merge this PR, could we open another PR to fix it?

@AndreMouche
Copy link
Contributor

/run-all-tests

@kennytm
Copy link
Collaborator Author

kennytm commented Mar 10, 2021

@AndreMouche well we need one more LGTM anyway 🙃

PTAL @glorv the latest commit

@glorv
Copy link
Collaborator

glorv commented Mar 10, 2021

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 LGTM1 label Mar 10, 2021
@ti-srebot ti-srebot added the status/LGT2 LGTM2 label Mar 10, 2021
@glorv
Copy link
Collaborator

glorv commented Mar 10, 2021

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 969b420 into pingcap:master Mar 10, 2021
ti-srebot pushed a commit to ti-srebot/br that referenced this pull request Mar 10, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #842

@kennytm kennytm deleted the test-only-mock branch March 10, 2021 09:56
@AndreMouche
Copy link
Contributor

It can not fix #837, sad. TiDB still can not work with the br's new master

@kennytm
Copy link
Collaborator Author

kennytm commented Mar 10, 2021

kennytm added a commit to ti-srebot/br that referenced this pull request Mar 10, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
kennytm added a commit to ti-srebot/br that referenced this pull request Mar 10, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
kennytm added a commit to kennytm/br that referenced this pull request Mar 16, 2021
kennytm added a commit to kennytm/br that referenced this pull request Mar 16, 2021
kennytm added a commit to kennytm/br that referenced this pull request Mar 16, 2021
kennytm added a commit to ti-srebot/br that referenced this pull request Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants