[Tests only] Flunk if test is not using SavepointTransaction

[nvasilevski]

Summary

Add checks to verify test setup before running it
On the main branch, assert_current_transaction_is_savepoint_transaction assertion is going to fail even though test expects to operate in the context of a SavepointTransaction

Other Information

I've been working on exploring some deadlock-related behaviours and these tests were very useful to reproduce some conditions. However I realized that even though names of those tests imply that we want to test against a savepoint transaction, in reality these tests were using RestartParentTransaction mainly because there was no queries to make the parent transaction dirty.

I would like to change this by actually performing some query and making the parent transaction dirty so the child transaction is going to be an instance of the savepoint transaction. Most likely I could have marked transaction as dirty manually but decided to use a bit higher-level approach as a real application would use by making a query.

To prevent similar situation in the future I decided to add flunk in case if nested transaction is not a savepoint transaction.

[jonathanhefner]

I haven't fully understood the context, but I wanted to offer some stylistic comments. (Feel free to dismiss them!)

[jonathanhefner]

Would it be suitable to factor these out as the following?

# This should cause the next nested transaction to be a savepoint transaction.
def make_parent_transaction_dirty
  Sample.take
end

Or would that defeat "higher-level approach as a real application would use by making a query"?

[nvasilevski]

Or would that defeat "higher-level approach as a real application would use by making a query"?

Not at all, it's actually a good idea, from my point of view extracting it into a method would keep all the context I wanted to share but by making it more readable. Thanks! I'll make a change

[jonathanhefner]

Could this also be suitably named assert_in_savepoint_transaction or assert_current_transaction_is_savepoint_transaction?

[nvasilevski]

Definitely, but do you think it's still reasonable to use flunk? I added an assertion first, but I feel like conceptually assert should be used for actual expectations of the test and flunk is more suitable to fail test in case if test setup is wrong

[jonathanhefner]

I hadn't considered that distinction! I'm not sure if Rails makes that distinction elsewhere, but it sounds reasonable to me.

[nvasilevski]

This tests started to fail after this change which either means that the expectation is incorrect or the actual Rails behaviour is incorrect. I'm going to dig into this but meanwhile I've created a separate PR for mysql changes only - #44690

[nvasilevski]

Hey folks, sorry for the draft-pr dance. I've updated changes, now it only contains postgresql changes as mysql ones were merged in a separate PR
I was using an unsuitable query in make_parent_transaction_dirty that was causing issues unrelated to what the test is trying to verify

[jonathanhefner]

I still don't have a full picture, but based on #44690, this looks good to me. 👍 (I can only merge documentation PRs though, so you may want to ping Rafael or Aaron.)

[jonathanhefner]

I was just curious: is there is a benefit to writing it this way instead of using assert_kind_of? If assert_kind_of fails and dumps the transaction object, is the output obnoxiously large?

[nvasilevski]

Not much benefit, to be honest. I just felt like I really wanted to make a clear distinction between what test asserts and some setup-verification checks. I feel like I'm bad at explaining so let me provide a short example, let's consider following tests:

test "premium plan users can access exclusive content" do
  user = users(:user_1)
  content = contents(:content_1)
  assert_predicate user.plan, :premium?
  assert_predicate content, :exclusive? 

  assert AccessCheck.for(user).can_access?(content)
end

I feel a little uncomfortable having 3 assertions when the test is intended to test only one thing. The minitest reporter also going to show 3 assertions which is slightly misleading. What I really wanted to do is just to verify that my setup is correct and whatever happens with user_1 or content_1 is not going to change the test setup without being noticed. So I would prefer something like:

test "premium plan users can access exclusive content" do
  user = users(:user_1)
  content = contents(:content_1)
  flunk("not a premium user") unless user.plan.premium?
  flunk("not an exclusive content") unless content.exclusive?

  assert AccessCheck.for(user).can_access?(content)
end

And I'm hoping that this makes a clear separation between test setup and test assertions.
However I agree that this is something I personally prefer and it doesn't seem to be widely used or used in Rails. And also this example is oversimplified, I guess I could have called fixtures like :premium_user and :exclusive_content to reduce chances of those fixtures behave differently. Those fixtures suppose to represent more complex real-world examples, like initialization of SavepointTransaction which is happening behind the scenes and it's better to be sure that we do it in the test setup, otherwise we will be testing different scenario

[jonathanhefner]

I understand what you mean -- I often write similar checks! I typically use assert_* to have more detailed errors (e.g. dumping actual values), but I think your way works as well.

My comment was more about inside the helper method. In other words, whether we name the method assert_current_transaction_is_savepoint_transaction or flunk_unless_current_transaction_is_savepoint_transaction (or verify_current_transaction_is_savepoint_transaction if you prefer?), it could use assert_kind_of internally to report the actual class in case of a failure. ...Unless the object dump would be obnoxiously large.

But it's not a big deal either way. 😃

[nvasilevski]

You are right about the output, I just tried with a dummy test like:

    test "is not a savepoint transaction" do
      Sample.transaction do
        assert_kind_of(ActiveRecord::ConnectionAdapters::SavepointTransaction, Sample.connection.current_transaction)
      end
    end

and the output is not the most actionable:

I could have passed message argument to the assert_kind_of method but one more thing I'm slightly concerned about is that assert_kind_of is counted as an assertion, but semantically I don't want it to be an assertion, my intention was just to verify test setup and not making any assertion just yet

I know that these reports are probably not that important but I'm working on a side project that utilizes these reports and I want to keep them as reliable as possible :)

[matthewd]

Thanks for identifying this! In #44526 I had broadly relied upon existing nesting tests like these to prove that RestartParent was functionally equivalent to a savepoint, but did not consider that I could be flattening some test scenarios from (base transaction vs savepoint) to (base transaction vs slightly decorated base transaction).

It took me quite a bit of staring to understand the significance of Bit.take here (or, the significance of not using Sample.take)... I think that's worth a comment inside make_parent_transaction_dirty.

Interestingly, it occurs to me that before your change, the before.wait on line 56 wasn't actually doing what it appeared: while the written intent was clearly to have both connections start their transactions before proceeding to the conflicting update, neither would actually materialize until the create. (Further thought—and the lack of flakiness from this test—confirms the after.wait is sufficient to guarantee the serialization failure we want.) I wonder if that's a wider point worthy of exploration... and hope it's not hiding further instances of not-testing-what-we-intended. That seems less likely than RestartParent-specific surprises, though: deferred materialization was added in 6.0 (#32647), so it's had much longer to settle.

[nvasilevski]

the significance of not using Sample.take)... I think that's worth a comment inside make_parent_transaction_dirty.

Yeah, I agree, it's just to be honest, I'm not sure I fully understand it and may provide a wrong or just vague explanation. Would you mind sharing your opinion on how to clearly explain why we are not using Sample.take here, even though almost "any" query should make transaction dirty? My understanding is that this SELECT query could potentially return two different results, a row with value: 1 (the original one) or a row with value: 2 (updated by another transaction with update_all`), depending on order in which these transactions are being executed. Which basically violates serializable isolation level and causes a serialization error. Even if this explanation is correct I'm not sure how to put it in the comment 🙃

Further thought—and the lack of flakiness from this test—confirms the after.wait is sufficient to guarantee the serialization failure we want.

Oh, hm, this is a good point, I think you are right and perhaps we could simplify this test in a separate PR? I mean, this is a tiny change but these tests are not the easiest to read so even small cleanup could help with readability

[matthewd]

My take (heh) is that the bodies of these tests are coordinating a controlled sequence of accesses to rows in samples, under serializable isolation; we need to run a query to dirty our transaction, but must avoid touching samples because otherwise our no-op query becomes an active participant.

perhaps we could simplify this test in a separate PR?

Yeah, possibly. I'm a bit torn, really: on one hand, it seems silly to have extra complicating coordination that isn't needed. But on the other hand, when trying to read this sort of deliberate-lockstep-progression simulated-concurrency test, it can be easier to read when there are more common touch-points: the fewer statements between each synchronization of the threads, the smaller the cross-product of possible sequencing combinations that might occur. 🤷🏻‍♂️

[nvasilevski]

Hey @matthewd, I've updated the comment, appreciate your insight!
Feel free to let me know if you think we can improve it or there is something else we might want to change about these tests. Thanks!

[nvasilevski]

Also one thing I keep thinking about but don't have any reason to do that, any chance that we may want to use a "write" query to make our transaction dirty, instead of doing SELECT

[matthewd]

Thanks!

any chance that we may want to use a "write" query to make our transaction dirty

In a theoretical future, I could imagine some read queries not dirtying the transaction (at least outside of stricter isolation levels)... but I think the effort involved in proving that safe might be nontrivial. And thanks to your added assertions, we're safe either way: if the implementation does change such that take is no longer sufficient, the test will fail, and we'll be able to redefine make_parent_transaction_dirty to suit at that point. 👍