Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miraradeva)

-- commits line 4 at r1:
nit: in our git commit message style guide, we suggest wrapping commit messages at 72 characters.

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 674 at r1 (raw file):

}

// maybeIncompatibleWithRequest checks if the TxnCoordSender is compatible with a given BatchRequest

maybeIncompatibleWithRequest doesn't make it particularly clear what this method does. Consider a name like maybeRejectIncompatibleRequest, which also benefits from symmetry with maybeRejectClientLocked.

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 674 at r1 (raw file):

}

// maybeIncompatibleWithRequest checks if the TxnCoordSender is compatible with a given BatchRequest

nit: In our Go coding guidelines, we suggest wrapping comments at 80 characters.

We also require full sentences and punctuation.

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 679 at r1 (raw file):

	ctx context.Context, ba *kvpb.BatchRequest,
) *kvpb.Error {
	if ba.IsLocking() && tc.typ == kv.LeafTxn {

We expect the typ check to be cheaper than the IsLocking check, because the latter needs to iterate over all requests in the batch. So let's switch the order of the short-circuiting.

Beyond that, we can restructure this code to be more extensible. For instance, consider a structure like:

switch tc.typ {
case kv.RootTxn:
    return nil
case kv.LeafTxn:
    if ba.IsLocking() {
        ...
    }
    return nil
default:
    panic("unexpected TxnType")
}

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go line 3023 at r1 (raw file):

}

// TestTxnCompatibleWithBatchRequest tests if a transaction and a batch request are compatible

Same note about capitalization and punctuation here and below.

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go line 3023 at r1 (raw file):

}

// TestTxnCompatibleWithBatchRequest tests if a transaction and a batch request are compatible

Could we rename this to TestTxnTypeCompatibleWithBatchRequest? Otherwise, this name is pretty ambiguous. There are many reasons why a batch request might be incompatible with a transaction.

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go line 3037 at r1 (raw file):

	// a LeafTxn is not compatible with a locking request
	_, err := leafTxn.GetForUpdate(ctx, roachpb.Key("a"))
	require.Error(t, err)

Could we also assert that the error is what we expect? require.Regexp is the tool for this.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go line 3023 at r1 (raw file):

That makes sense. I'm wondering what the guidelines are around unit tests for specific functions, especially in this case where the function name is more general than what it does initially. E.g. the function name suggests we may reject a Batch Request if it's not compatible with the TxnCoordSender, but the specific initial compatibility rule is only if the txn is leaf and the request is locking. We set up the function in such a way that it's extensible to other types of incompatibility. Do we:
(a) name the test "Test+genericFuntionName" to exactly match the broader function name and test the specific behavior, or (b) name the test "Test+specificBehavior", which doesn't match the function name, and again only tests the specific behavior? I've seen (a) recommended in some Go tutorials, and it might make it easier to know which function this test applies to, but (b) might make more sense because the function is private, and we only care about the specific behavior change it introduced.

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @miraradeva)

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go line 3023 at r1 (raw file):

I don't think there are any hard and fast rules for this kind of thing. In general, Go encourages unit tests to be extensible to variants, as you say. Support for subtests encourages this kind of thing. On the other hand, it's helpful to scope down the possible conditions a test could be exercising so that the role of the test can be understood by readers and by would-be extenders.

So in this case, I think claiming that the test is responsible for exercising compatibility between transactions and batch requests is too broad, but claiming that it tests the compatibility between specific TxnTypes and batch requests (the analog to maybeRejectIncompatibleRequest) is appropriate.

On that note, consider s/if a transaction/if a transaction type/.