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

Fix bug when call XAResource.start with TMJOIN flag, the old localAutoCommitMode lost #434

Merged
merged 1 commit into from Dec 3, 2015

Conversation

Projects
None yet
3 participants
@chunlinyao
Contributor

chunlinyao commented Nov 26, 2015

No description provided.

@@ -341,6 +341,29 @@ public void testRestoreOfAutoCommit() throws Exception {
xaRes.commit(xid, true);
assertTrue(!conn.getAutoCommit());
// Test true case
conn.setAutoCommit(true);

This comment has been minimized.

@vlsi

vlsi Nov 26, 2015

Member

please use spaces, not tabs

This comment has been minimized.

@vlsi

vlsi Nov 26, 2015

Member

Please use separate test methods. Avoid putting several tests into a single method.
Having separate methods helps executing tests one by one.

assertTrue(conn.getAutoCommit());
// Test with TMJOIN
conn.setAutoCommit(true);

This comment has been minimized.

@vlsi

vlsi Nov 26, 2015

Member

no tabs please

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 26, 2015

Looks good

xaRes.end(xid, XAResource.TMSUCCESS);
xaRes.commit(xid, true);
assertTrue(conn.getAutoCommit());

This comment has been minimized.

@vlsi

vlsi Nov 26, 2015

Member

Please, avoid assertTrue/assertFalse: use assertEquals and provide human-readable message.
For instance: "XaResource should have restored connection autocommit mode after commit of the transaction to the initial state".
In other words: good test should produce a message that is good enough to copy-paste into a bug report.

With assertTrue all you get is a stacktrace and if such a test fails a developer has to reverse engineer the intention behind that code.

This comment has been minimized.

@davecramer

davecramer Nov 26, 2015

Member

On 26 November 2015 at 06:37, Vladimir Sitnikov notifications@github.com
wrote:

In org/postgresql/test/xa/XADataSourceTest.java
#434 (comment):

@@ -341,6 +341,29 @@ public void testRestoreOfAutoCommit() throws Exception {
xaRes.commit(xid, true);

     assertTrue(!conn.getAutoCommit());
  •    // Test true case
    
  • conn.setAutoCommit(true);
  •    xid = new CustomXid(15);
    
  •    xaRes.start(xid, XAResource.TMNOFLAGS);
    
  •    xaRes.end(xid, XAResource.TMSUCCESS);
    
  •    xaRes.commit(xid, true);
    
  •    assertTrue(conn.getAutoCommit());
    

Please, avoid assertTrue/assertFalse: use assertEquals and provide
human-readable message.
For instance: "XaResource should have restored connection autocommit mode
after commit of the transaction to the initial state".
In other words: good test should produce a message that is good enough to
copy-paste into a bug report.

With assertTrue all you get is a stacktrace and if such a test fails a
developer has to reverse engineer the intention behind that code.

Might be a good idea to put this in the developer notes ?

Dave Cramer

@chunlinyao chunlinyao force-pushed the chunlinyao:master branch from f676e48 to df09e2b Nov 27, 2015

@chunlinyao

This comment has been minimized.

Contributor

chunlinyao commented Nov 27, 2015

Removed tabs, added descriptions to assert method.

@vlsi

This comment has been minimized.

Member

vlsi commented Dec 3, 2015

looks good

davecramer added a commit that referenced this pull request Dec 3, 2015

Merge pull request #434 from chunlinyao/master
Fix bug when call XAResource.start with TMJOIN flag, the old localAutoCommitMode lost

@davecramer davecramer merged commit 153cb92 into pgjdbc:master Dec 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment