Add TransactionFactory, Transaction test cases #3277
Conversation
|
Hi, thanks your advice, I have rename base test class.
|
|
I think you may have misunderstood me. The class 'BaseTransactionTest', I don't want to see 'Test' in the class name. That is what surefire uses to tell it to scan for tests. There are no tests in that class. Its an interface you are using for your other tests to inherit. Thus it is a base class, so what I meant was try something like 'TransactionBase' instead of 'TransactionTest'. Same with the other base class. Does that make sense? Basic reason is that there is no reason for surefire to scan those 2 interfaces / abstract classes as nothing in there to process. Its the real tests that are tested plus naming this way makes it clear. Otherwise I'd think there were tests in there. |
|
Sorry, i will change the class name as you expcted. |
|
|
||
| @Test | ||
| @Override | ||
| public void shouldCommit() throws SQLException { |
There was a problem hiding this comment.
In the case of using inheritance here, the reader of this test would require extensive knowledge of internals. In other words, shouldCommit in this case verifies that commit is never called, which can be quite confusing, and might even be seen as an incorrect test case. So not sure if using the inheritance here is a great idea.
Either it should be renamed, or a class level comment explaining this should be added imo.
There was a problem hiding this comment.
Thank you! I have found the problem, because ManagedTransaction do not manage connection's rollback and commit, so it will never call commit and rollback method in Connection class.
|
|
||
| transactionFactory.setProperties(properties); | ||
|
|
||
| assertTrue((Boolean) getValue(transactionFactory.getClass().getDeclaredField("skipSetAutoCommitOnClose"), transactionFactory)); |
There was a problem hiding this comment.
Imo, using constructs like assertTrue, or assertEquals(true, xxx) does not make it that clear what went wrong initially in test output.
It's more a personal thing, but I prefer using assertj, as the failure messages are more descriptive. Not sure what others think.
There was a problem hiding this comment.
I'm ok with how this is for now. Yes assertj makes it a lot more readable. Possibly we can run open rewrite against this for tests to clean them up. Will check into that separately.
|
@hazendaz Hi, I have changed the test class name. If you find more issues, i will fix it. Thanks. |
|
Will be a few days but will get to this. |
|
merge conflicts need to be resolved before this can progress. |
# Conflicts: # src/test/java/org/apache/ibatis/scripting/xmltags/ChooseSqlNodeTest.java # src/test/java/org/apache/ibatis/scripting/xmltags/ForEachSqlNodeTest.java # src/test/java/org/apache/ibatis/scripting/xmltags/MixedSqlNodeTest.java
Hi, I have resolved merge conflicts. |
|
@mawen12 coverage provided by this is likely none. While it shows +0.1% that is within normal bounds for it changing just depending on the build. Going to merge it otherwise. There are some formatting issues so those will get applied after. Next time around, do make sure to run mvn clean install before submitting to ensure all the plugins run. |
Supplement the missing unit test cases for
TransactionFactory,Transaction.