-
Notifications
You must be signed in to change notification settings - Fork 8.8k
test: Improve the test case coverage of [sqlparser] module to 70% #6608
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
Conversation
@slievrly @funky-eyes Hi, can you review my PR? |
sqlparser/seata-sqlparser-core/src/test/java/org/apache/seata/sqlparser/EscapeSymbolTest.java
Outdated
Show resolved
Hide resolved
@@ -161,6 +161,7 @@ Add changes here for all PR submitted to the 2.x branch. | |||
- [[#6375](https://github.com/apache/incubator-seata/pull/6375)] override console nested dependencies | |||
|
|||
### test: | |||
- [[#6608](https://github.com/apache/incubator-seata/pull/6608)] add unit test for sql-parser-core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add it to zh-cn/2.x.md
...ser/seata-sqlparser-core/src/test/java/org/apache/seata/sqlparser/struct/ColumnMetaTest.java
Outdated
Show resolved
Hide resolved
...rser/seata-sqlparser-core/src/test/java/org/apache/seata/sqlparser/struct/IndexMetaTest.java
Outdated
Show resolved
Hide resolved
...rser/seata-sqlparser-core/src/test/java/org/apache/seata/sqlparser/struct/TableMetaTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ok, let's run the CI workflow once to see the outcome. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #6608 +/- ##
============================================
+ Coverage 50.74% 50.88% +0.13%
- Complexity 5709 5832 +123
============================================
Files 1035 1051 +16
Lines 35863 36318 +455
Branches 4257 4314 +57
============================================
+ Hits 18199 18479 +280
- Misses 15843 15985 +142
- Partials 1821 1854 +33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage 51.13% 49.52% -1.61%
Unit test coverage did not meet expectations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our goal is to improve test coverage to 70%,can you add more? thanks for your job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I will merge this PR and look forward to you continuing to increase the unit test coverage for this module
Ⅰ. Describe what this PR did
fix #6508
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews