-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Decouple unit test code from business code #17623
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #17623 +/- ##
=======================================
Coverage ? 45.49%
=======================================
Files ? 798
Lines ? 88916
Branches ? 0
=======================================
Hits ? 40452
Misses ? 41965
Partials ? 6499
Continue to review full report at Codecov.
|
lunny
approved these changes
Nov 12, 2021
12a5117
to
2cd3bd9
Compare
KN4CK3R
approved these changes
Nov 12, 2021
Chianina
pushed a commit
to Chianina/gitea
that referenced
this pull request
Mar 28, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
lgtm/done
This PR has enough approvals to get merged. There are no important open reservations anymore.
skip-changelog
This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
type/refactoring
Existing code has been cleaned up. There should be no new functionality.
type/testing
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many unit test code and business code were coupled together.
This PR decouples them.
After this refactoring, we only import
testing
andassert
in unit test code:Some details:
What are
Tester
andAsserter
?They are intermediate interfaces for this refactoring. There are many assertions in
models.CheckConsistencyFor
(which callsdb.AssertXxx
), so we introduce theTester
/Asserter
interfaces to decoupleCheckConsistencyFor
from testing frameworks. Then we do not need to import any testing frameworks for non-unit-test code. The benefit is that we get a clearer and smaller built binary.Why don't we refactor
CheckConsistencyFor
directly to get rid of the assertions?It will be a huge project. There are too many related code of
CheckConsistencyFor
, which may cause cycle-import problems. The refactoring ofCheckConsistencyFor
is very difficult to be done in a few PRs. So we now we just make the first step to decouple it from testing framework, and then we can refactorCheckConsistencyFor
and related models one by one.Then, one day, if
CheckConsistencyFor
is clean enough, we can remove the intermediate interfaces (Tester
/Asserter
).