chore(dashboard/tests): ensure any DB changes are rolled back #70189

artem-vavilov · 2026-01-06T14:46:30Z

Wraps setup_all callbacks in a class-level DB transaction (rolled back in teardown_all) so records created in setup_all cannot leak across tests, without relying on use_transactional_test_case

Links

Copilot

Pull request overview

This PR refactors the transactional test case implementation to ensure database changes are automatically rolled back after tests. The key change is moving from an opt-in use_transactional_test_case class attribute to leveraging Rails' built-in use_transactional_tests feature, which is now explicitly enabled by default in TransactionalTestCase.

Key Changes:

Simplified TransactionalTestCase module to use Rails' built-in transactional test support
Modified setup_all to wrap its block in a database transaction that's rolled back in teardown_all
Removed all manual self.use_transactional_test_case assignments across the codebase since transactional behavior is now enabled by default

Reviewed changes

Copilot reviewed 121 out of 121 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
dashboard/test/testing/transactional_test_case.rb	Core refactoring: simplified to use Rails' `use_transactional_tests` and wrapped `setup_all` blocks in transactions
dashboard/test/testing/test_case_test.rb	Added test coverage for the new transaction behavior in `setup_all`
dashboard/test/models/*.rb (45 files)	Removed redundant `use_transactional_test_case` assignments
dashboard/test/lib/*.rb (21 files)	Removed redundant `use_transactional_test_case` assignments
dashboard/test/controllers/*.rb (43 files)	Removed redundant `use_transactional_test_case` assignments
dashboard/test/integration/*.rb (3 files)	Removed redundant `use_transactional_test_case` assignments
dashboard/test/helpers/*.rb (2 files)	Removed redundant `use_transactional_test_case` assignments
dashboard/test/models/pd/application/principal_approval_application_test.rb	Changed `setup_all` to `setup` (no longer needs transaction wrapping)
dashboard/test/lib/services/complete_application_reminder_test.rb	Changed `setup_all` to `setup` (no longer needs transaction wrapping)
dashboard/test/lib/pd/survey_pipeline/mapper_test.rb	Changed `setup_all` to `setup` (data setup doesn't require persistence)
dashboard/test/lib/pd/survey_pipeline/daily_survey_joiner_test.rb	Changed `setup_all` to `setup` (data setup doesn't require persistence)
dashboard/test/controllers/api/v1/pd/teacher_attendance_report_controller_test.rb	Consolidated `setup_all` time travel into `setup` block

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

artem-vavilov · 2026-01-06T18:11:16Z

dashboard/test/testing/transactional_test_case.rb

-
-        setup_all do
-          if use_transactional_test_case?
-            setup_fixtures


#70169 (comment)

davidsbailey · 2026-01-06T19:06:51Z

this could be worth trying, but I am concerned that use_transactional_tests might not be protecting us in the way that we want, because I was able to show here that without use_transactional_test_case, there would be collisions between objects across test cases: f7a0d43 . if use_transactional_tests does what it says, then why didn't it protect us in that case?

if you want to continue down this path, my recommendation would be to do a similar test to the commit linked above, as proof that we are cleaning up between tests. because we run tests 7 ways parallel in drone, I would recommend adding the colliding objects to at least 10 different setup_all blocks.

artem-vavilov · 2026-01-06T20:12:40Z

this could be worth trying, but I am concerned that use_transactional_tests might not be protecting us in the way that we want, because I was able to show here that without use_transactional_test_case, there would be collisions between objects across test cases: f7a0d43 . if use_transactional_tests does what it says, then why didn't it protect us in that case?

if you want to continue down this path, my recommendation would be to do a similar test to the commit linked above, as proof that we are cleaning up between tests. because we run tests 7 ways parallel in drone, I would recommend adding the colliding objects to at least 10 different setup_all blocks.

use_transactional_tests opens a transaction for each test case (the test or it blocks, d.f test_..., etc.), while setup_all runs earlier at the class level and wraps all test cases in the class.

So use_transactional_tests basically covers that part:

code-dot-org/dashboard/test/testing/transactional_test_case.rb

Lines 30 to 38 in 3f113e3

    
           def before_setup 
        
             begin_transaction 
        
             super 
        
           end 
        
           def after_teardown 
        
             super 
        
             rollback_transaction 
        
           end

But we still need to cover this one:

code-dot-org/dashboard/test/testing/transactional_test_case.rb

Lines 18 to 27 in 3f113e3

    
           setup_all do 
        
             if use_transactional_test_case? 
        
               setup_fixtures 
        
               begin_transaction 
        
             end 
        
           end 
        
           teardown_all do 
        
             rollback_transaction if use_transactional_test_case? 
        
           end

artem-vavilov · 2026-01-06T20:17:45Z

we run tests 7 ways parallel in drone

This does not really matter, since parallel tests run across isolated worker processes with separate test databases while still executing tests sequentially within each worker

davidsbailey · 2026-01-06T20:27:03Z

we run tests 7 ways parallel in drone

This does not really matter, since parallel tests run across isolated worker processes with separate test databases while still executing tests sequentially within each worker

that may be, but the number of separate test databases is also 7. if you add 7 colliding objects to fewer than 7 setup_all methods, then you run the risk that each one pollutes exactly one test db without ever having a collision.

artem-vavilov · 2026-01-06T20:28:12Z

my recommendation would be to do a similar test to the commit linked above, as proof that we are cleaning up between tests.

The existing TransactionTest already verifies database isolation by establishing a baseline, making a change in one test, and then confirming in a subsequent test that the change was rolled back, but I can add more tests if needed.

davidsbailey

Thanks for following up on this Artem! It looks promising, but I think some explicit manual validation is needed to prove that this does the right thing. The following seems like it would be adequate to me:

introduce some colliding object names, as illustrated in previously linked commit
do some print debugging to prove that when running a single test file, the setup_all method in the test file is being executed after the database transaction

davidsbailey · 2026-01-06T18:40:33Z

dashboard/test/controllers/code_reviews_controller_test.rb

-  # Setting this to true causes some weird db locking issue, possibly due to
-  # some writes to the projects table coming from a different connection via
-  # sequel.
-  self.use_transactional_test_case = false


please keep a version of the comment explaining that any new failures due to db locking issues could be due to the reason described here

Please let me know if this warning is fine, or if you would prefer to keep the original comment:

code-dot-org/dashboard/test/testing/transactional_test_case.rb

Lines 21 to 24 in 2788061

# Ensures any database changes made in setup_all are rolled back after all tests in the class.

# @warning This keeps a transaction open for the whole test class.

# Writes from other connections, for example Sequel, can wait on locks or hit a MySQL deadlock.

private def setup_all(*args, &block)

there is not much in this setup_all block. how about consolidating it into the setup block instead? then the existing comment should be fine, since that should eliminate the known risk in this particular file.

Do you mean this?

that link didn't work for me, but the updated contents of CodeReviewsControllerTest looks good to me!

davidsbailey · 2026-01-06T20:47:11Z

my recommendation would be to do a similar test to the commit linked above, as proof that we are cleaning up between tests.

The existing TransactionTest already verifies database isolation by establishing a baseline, making a change in one test, and then confirming in a subsequent test that the change was rolled back, but I can add more tests if needed.

sorry I missed this earlier comment. I don't see anything in that test which covers the case where the colliding objects are created in setup_all, so it seems like this is covering the use_transactional_tests functionality rather than the now-removed use_transactional_test_case functionality. please let me know if I am missing something there -- if you have automated testing then I do not also need manual verification for the same functionality.

…sControllerTest

…tCase feature

artem-vavilov · 2026-01-07T18:32:04Z

sorry I missed this earlier comment. I don't see anything in that test which covers the case where the colliding objects are created in setup_all, so it seems like this is covering the use_transactional_tests functionality rather than the now-removed use_transactional_test_case functionality. please let me know if I am missing something there -- if you have automated testing then I do not also need manual verification for the same functionality.

Added some tests: 77994c4

… tests feature with custom transaction wrapper

…nSummaryPodcastsControllerTest

…tion query created by lazy transaction

davidsbailey · 2026-01-09T03:48:09Z

dashboard/test/testing/transactional_test_case_test.rb

+class ActiveSupport::Testing::TransactionalTestCaseTest < ActiveSupport::TestCase
+  UNIT_NAMES = Array.new(10) {Faker::Lorem.unique.word.downcase}
+
+  setup_all do


It looks like one of my comments is not showing up on the PR. Please double check that this call to setup_all runs when you expect it to, because it can sometimes have unexpected behavior when run with describe / it.

We already verify that the data and instance variables created in the root class setup_all callback are shared in the describe/context/it blocks, which is expected and desirable behavior. And that they run within a single transaction, meaning no extra transaction is created. Could you please specify what else you would like us to verify?

This reverts commit e86a5a2.

artem-vavilov added 5 commits January 6, 2026 16:45

chore(dashboard/tests): ensure any DB changes are rolled back

9b8a94a

fix(setup_all): skip executing block when none is given

70cf21d

chore(setup_all): test transaction opening

c231867

chore(dashboard/tests): disable pre_loaded_fixtures

e48bc4f

Merge remote-tracking branch 'origin/staging' into transactional-tests

b54fb92

artem-vavilov requested review from Copilot and davidsbailey January 6, 2026 17:23

Copilot AI reviewed Jan 6, 2026

View reviewed changes

artem-vavilov mentioned this pull request Jan 6, 2026

fix(dashboard/tests): setup fixtures for each transactional test case #70169

Merged

artem-vavilov commented Jan 6, 2026

View reviewed changes

davidsbailey reviewed Jan 6, 2026

View reviewed changes

chore(dashboard/tests): add warning for test class transaction

2788061

artem-vavilov force-pushed the transactional-tests branch from 7dad9f6 to ca3db3c Compare January 6, 2026 21:39

chore(dashboard/tests): eliminate transaction lock risk in CodeReview…

2420186

…sControllerTest

artem-vavilov force-pushed the transactional-tests branch from ca3db3c to 2420186 Compare January 6, 2026 21:49

chore(dashboard/tests): test ActiveSupport::Testing::TransactionalTes…

77994c4

…tCase feature

artem-vavilov requested a review from davidsbailey January 7, 2026 19:27

artem-vavilov added 4 commits January 7, 2026 23:10

chore(dashboard/tests): chore(dashboard/tests): replace transactional…

0e120c8

… tests feature with custom transaction wrapper

Merge remote-tracking branch 'origin/staging' into transactional-tests

a697f66

chore(dahsboard/tests): remove use_transactional_test_case in AiLesso…

e940f85

…nSummaryPodcastsControllerTest

fix(dashboard/tests): update ApiControllerTest to account for transac…

3c463e6

…tion query created by lazy transaction

artem-vavilov force-pushed the transactional-tests branch from f27c5ab to 3c463e6 Compare January 8, 2026 10:22

davidsbailey approved these changes Jan 9, 2026

View reviewed changes

davidsbailey reviewed Jan 9, 2026

View reviewed changes

artem-vavilov added 3 commits January 9, 2026 13:28

Merge remote-tracking branch 'origin/staging' into transactional-tests

4d0642a

chore(dashboard/tests): update TransactionalTestCaseTest

ba24d89

Merge remote-tracking branch 'origin/staging' into transactional-tests

17bc3f1

artem-vavilov requested a review from Copilot January 9, 2026 12:15

This comment was marked as off-topic.

Sign in to view

artem-vavilov added 2 commits January 9, 2026 15:12

chore(dahsboard/tests): trigger all tests

e86a5a2

Revert "chore(dahsboard/tests): trigger all tests"

5ba0e55

This reverts commit e86a5a2.

artem-vavilov merged commit a348065 into staging Jan 9, 2026
6 checks passed

artem-vavilov deleted the transactional-tests branch January 9, 2026 14:04

This was referenced Jan 9, 2026

chore(dashboard/tests): optimize fixtures loading #70171

Closed

fix(dahsboard/tests): enable use_transactional_tests #70271

Merged

	# Ensures any database changes made in setup_all are rolled back after all tests in the class.
	# @warning This keeps a transaction open for the whole test class.
	# Writes from other connections, for example Sequel, can wait on locks or hit a MySQL deadlock.
	private def setup_all(*args, &block)

chore(dashboard/tests): ensure any DB changes are rolled back #70189

chore(dashboard/tests): ensure any DB changes are rolled back #70189

Uh oh!

Conversation

artem-vavilov commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

artem-vavilov Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

davidsbailey commented Jan 6, 2026

Uh oh!

artem-vavilov commented Jan 6, 2026

Uh oh!

artem-vavilov commented Jan 6, 2026

Uh oh!

davidsbailey commented Jan 6, 2026

Uh oh!

artem-vavilov commented Jan 6, 2026

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

davidsbailey Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

artem-vavilov Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

davidsbailey Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

artem-vavilov Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

davidsbailey commented Jan 6, 2026

Uh oh!

artem-vavilov commented Jan 7, 2026

Uh oh!

davidsbailey Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

artem-vavilov Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

artem-vavilov commented Jan 6, 2026 •

edited

Loading

artem-vavilov Jan 6, 2026 •

edited

Loading