Skip to content

TestTransaction should implement Transaction instead of TransactionBase #1124

@ctubbsii

Description

@ctubbsii

Currently, TestTransaction implements TransactionBase, but still has a commit() and close() method with the same signature as the Transaction interface, which itself extends AutoCloseable. It should implement Transaction instead.

TestTransaction also has a done() method that does:

try {
  commit();
} finally {
  close();
}

Once TestTransaction is AutoCloseable, inheriting that from Transaction, the calls to done() can, and should, be converted to calls to commit() at the end of a try-with-resources block, and the done() method should be removed. This will make the lifecycle management of test transactions in the test code more clear, rather than relying on the implied close() method inside the done(). It will also make the order of commit() calls more clear when writing/maintaining test code. With commit() hidden inside the done() method, it's not immediately obvious when they are called.

This proposal would cause code written like:

    TestTransaction tx = new TextTransaction();
    // ... stuff here
    tx.done(); // implied commit() and close()

to be written more explicitly as:

    try (TestTransaction tx = new TextTransaction()) {
      // ... stuff here
      tx.commit(); // explicit commit()
    } // auto closed

This example doesn't look that much better, but it's a big improvement when there are multiple transactions being created, committed, and closed in a test.

Metadata

Metadata

Assignees

No one assigned

    Labels

    good first issueWell written issue that is very specific and easy to do, intended for first time contributors.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions