Skip to content

TestTransaction should implement Transaction instead of TransactionBase #1124

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.

Activity

  1. added
    good first issueWell written issue that is very specific and easy to do, intended for first time contributors.
    on Nov 16, 2022
  2. ctubbsii commented on Nov 17, 2022

    @ctubbsii
    MemberAuthor

    Note to potential contributors: this could be done in two smaller steps, as separate PRs.

    1. inline all instances of TestTransaction.done() (and remove the method), and change implements TransactionBase to implements Transaction (adding @Override annotations to close() and commit() would be nice, too), and
    2. track down TestTransaction declarations and convert them to use try-with-resources blocks instead of explicitly calling close()
  3. 555vedant commented on Oct 2, 2024

    @555vedant

    is this issue still active ??

  4. ctubbsii commented on Nov 26, 2024

    @ctubbsii
    MemberAuthor

    is this issue still active ??

    The issue is probably still relevant, as no action has been done to address it, but the project overall has become relatively inactive lately. You are nevertheless welcome to contribute a PR if you wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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