Hi Jeremy, I appreciate the reply.
> Technically, you don't know the same connection will available. There > could be a disconnect error the closes the connection, with the rest of the > code executing outside of a transaction on a new connection. It also can > result in pool timeouts in certain cases depending on access patterns. You're right, I haven't considered that. I would also override Dataset#connect after the connection has been > established to raise an error, to handle cases where there is a disconnect > error. Sounds like a good idea to address the above, thanks. However, this assumes that run_request doesn't return until all database > access has completed on the server. Maybe there are cases where that isn't > true. Yeah, I was thinking that at least with a SPA-type of application, where you need to explicitly wait with Capybara for new pages to load after clicking links or submitting forms, that it could happen that test code tries to access the database during request processing. I don't think the main advantage is performance, I think the main advantage > is ease of use. With the truncation/deletion method, you need to > explicitly undo any changes you are making during the test (figuring out > which tables are being modified), instead of having that implicitly handled > by the transaction. There may be performance advantages, especially with a > large number of tables, or with significantly latency to the database. Makes sense. The DatabaseCleaner gem should hopefully solve most of this complexity, but it is an additional dependency. Though I would still think that's better than this <https://github.com/rails/rails/blob/main/activerecord/lib/active_record/test_fixtures.rb>, but I guess that's how Active Record maintainers prefer it :P Just to be clear, I'm not saying what you are doing in regards to > connection sharing will not work. I just am hesitant to accept it as part > of Sequel's documentation until I can prove to myself that it is safe. > Have you used this approach in a decently sized application with positive > results? In our Rails app we're currently only using Sequel for writing data into a time-series database (querying this data is done by another tool), but I'll be sure to try out this approach when we need it with browser tests and let you know. Kind regards, Janko On Sun, May 16, 2021 at 10:45 PM Jeremy Evans <[email protected]> wrote: > On Sun, May 16, 2021 at 1:55 AM Janko Marohnić <[email protected]> > wrote: > >> From my understanding, releasing the connection back into the pool after >> starting the outer transaction allows the test & app threads to use the >> same connection in a thread-safe way, because additional calls to >> Database#synchronize will correctly lock the connection for the duration of >> queries. I don't see any risk here, because we know the same connection >> will be available when the test transaction should be rolled back. We >> should probably acquire the connection back, though, to ensure the app >> thread isn't using it at the moment of rollback. >> > > Technically, you don't know the same connection will available. There > could be a disconnect error the closes the connection, with the rest of the > code executing outside of a transaction on a new connection. It also can > result in pool timeouts in certain cases depending on access patterns. > > >> DB.transaction(rollback: :always, auto_savepoint: true) do >> DB.pool.send(:release, Thread.current) >> spec.run >> DB.pool.send(:acquire, Thread.current) >> end >> > > This does look safer. I would also override Dataset#connect after the > connection has been established to raise an error, to handle cases where > there is a disconnect error. > > There is still risk that the app thread would continue executing queries >> in a request that still hasn't finished before the end of the test, which >> could cause data to get written outside of a transaction, but there is >> probably ways to work around that as well. I would generally like to >> explore whether it's possible to make browser tests work with transactional >> tests in a safe way, because that's the level of convenience that Rails >> system tests provide. >> > > While it may not be possible to support all possible cases, it may be > possible to support the most common cases. > > >> A single-threaded pool wouldn't work for me in this case, because of the >> lack of locking around connection access, which would almost certainly >> result in race conditions. I've experienced >> <https://github.com/janko/sequel-activerecord_connection/issues/12> >> these when working on the sequel-activerecord_connection gem, and I don't >> know how I would avoid them with a single-threaded pool. >> > > I guess it depends on what the access patterns are. Unless there is some > sort of asynchronous behavior, I would assume usage such as: > > it "some spec" do > interact_with_database > run_request > interact_with_database > run_request > interact_with_database > run_request > interact_with_database > end > > Would be fine with the single threaded pool. However, this assumes that > run_request doesn't return until all database access has completed on the > server. Maybe there are cases where that isn't true. > > Yes, my current code is using private API, but it might be worth the >> trade-off to gain better performance. Though I'm still yet to verify >> whether the truncation approach really does impact test suite performance >> significantly. If it's not, then I don't understand why did Active Record >> maintainers go to such great lengths >> <https://github.com/rails/rails/blob/61fb58f6a7f852d765d616d1da41d17851ec7afc/activerecord/lib/active_record/test_fixtures.rb> >> to make transactional tests work with Capybara browser tests. >> > > I don't think the main advantage is performance, I think the main > advantage is ease of use. With the truncation/deletion method, you need to > explicitly undo any changes you are making during the test (figuring out > which tables are being modified), instead of having that implicitly handled > by the transaction. There may be performance advantages, especially with a > large number of tables, or with significantly latency to the database. > > Just to be clear, I'm not saying what you are doing in regards to > connection sharing will not work. I just am hesitant to accept it as part > of Sequel's documentation until I can prove to myself that it is safe. > Have you used this approach in a decently sized application with positive > results? > > Personally, I avoid this issue as much as possible by avoiding javascript > as much as possible, and using transactions with rack-test for the vast > majority of my web testing. For the tests that require javascript, I use > non-transactional tests. However, I realize that not all developers have > that luxury. > > Thanks, > Jeremy > > -- > You received this message because you are subscribed to a topic in the > Google Groups "sequel-talk" group. > To unsubscribe from this topic, visit > https://groups.google.com/d/topic/sequel-talk/lnZFBo4XHII/unsubscribe. > To unsubscribe from this group and all its topics, send an email to > [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/sequel-talk/CADGZSSf1XC427SQ2j3iOTYAX5d53tWwhiKOOTffG71U6wf%3DnUA%40mail.gmail.com > <https://groups.google.com/d/msgid/sequel-talk/CADGZSSf1XC427SQ2j3iOTYAX5d53tWwhiKOOTffG71U6wf%3DnUA%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/sequel-talk/CAJ8a-RNvtLmV1CsHY3Ox7wo43wFJDf%3D9mc7UDRzvrKwo8%2BVPkg%40mail.gmail.com.
