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.
|
@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. |
Supplement the missing unit test cases for
TransactionFactory,Transaction.