Skip to content

use transactional test case whenever we use setup_all #70001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
davidsbailey merged 5 commits into from
Dec 16, 2025
Merged

use transactional test case whenever we use setup_all #70001

davidsbailey merged 5 commits into from
Dec 16, 2025

Conversation

@davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Dec 11, 2025 •
edited
Loading

An unexplained dashboard unit test failure prompted me to investigate a hunch about using setup_all without use_transactional_test_case. as you can see from the first drone run, we are not properly cleaning up objects inside setup_all if use_transactional_test_case is not also specified. in that commit, I added some intentionally colliding create commands, and we get errors like this:

==ERROR["setup_all", Services::CompleteApplicationReminderTest, 188.73090645000002]
 setup_all#Services::CompleteApplicationReminderTest (188.73s)
Minitest::UnexpectedError:         ActiveRecord::RecordInvalid: Validation failed: Name has already been taken

plus many more errors and failures which cascade from these.

The short-term fix is to add use_transactional_test_case wherever we use setup_all, which fixes the above failures.

I generated the list of files to update with this command:

[14:15:22] Mac:~/src/cdo/dashboard/test (always-use-transactional-test-case-with-setup-all)$ grep -l -w setup_all **/*_test.rb | xargs grep -L use_transactional_test_case

Testing story

  • see drone results on individual commits for evidence that the problem exists and that this solution fixes it

Follow-up work

  • it would be nice to have a lint rule to enforce that we use_transactional_test_case whenever we use setup_all

@davidsbailey davidsbailey force-pushed the always-use-transactional-test-case-with-setup-all branch from 156f5ac to f7a0d43 Compare December 11, 2025 23:32
@davidsbailey davidsbailey requested a review from sureshc December 15, 2025 04:43
@davidsbailey davidsbailey marked this pull request as ready for review December 15, 2025 04:43
@etaderhold
Copy link
Contributor

etaderhold commented Dec 15, 2025

Thanks for looking into this! I'm curious if it's possible to maybe patch something in our test stack so that setup_all automatically triggers the transactional behavior. If so that seems nicer to me than a lint rule that would nudge people to do it manually.

@alex-m-brown alex-m-brown changed the base branch from staging-next to staging December 15, 2025 15:25
@davidsbailey
Copy link
Member Author

davidsbailey commented Dec 16, 2025

Thanks for looking into this! I'm curious if it's possible to maybe patch something in our test stack so that setup_all automatically triggers the transactional behavior. If so that seems nicer to me than a lint rule that would nudge people to do it manually.

@etaderhold I think this is worth looking into! I'm fairly confident I know how to do this with a lint rule -- I have a couple of ideas for how this might be possible by setting use_transactional_test_case automatically based on the presence of setup_all in the test case, which I'd be willing to try when I get to this. However, in part due to the following weird logic, my confidence is a bit low in the viability of that approach:

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

Lines 14 to 27 in b9c150b

# It's not 100% clear why this is necessary, but here's what we do know:
#
# Between Rails 5.0 and 5.2, the implementation of ActiveSupport callbacks was refactored to
# improve the backtrace experience (https://github.com/rails/rails/pull/26147). This
# refactoring made some changes to the way the callbacks are stored on the class, and this
# change seems to have resulted in our extension not being correctly applied to descendant
# classes. Specifically, we found ourselves in a situation where these callbacks were
# defined correctly on the ActiveSupport:TestCase class, but only on that class.
#
# Our naive solution is to explicitly define the callbacks on all descendant classes here at
# inclusion time, rather than trying to rely on class inheritance as we did before.
([self] + ActiveSupport::DescendantsTracker.descendants(self)).reverse_each do |target|
target.define_callbacks :setup_all, :teardown_all
end

In any case I'd like to leave that investigation as separate from this PR.

@davidsbailey davidsbailey merged commit 1339cab into staging Dec 16, 2025
4 checks passed
@davidsbailey davidsbailey deleted the always-use-transactional-test-case-with-setup-all branch December 16, 2025 22:53
@davidsbailey davidsbailey mentioned this pull request Dec 18, 2025
Closed
davidsbailey added a commit that referenced this pull request Dec 18, 2025
@davidsbailey
Revert "use transactional test case whenever we use setup_all (#70001)"
44b69d2 
This reverts commit 1339cab.
@davidsbailey davidsbailey mentioned this pull request Dec 18, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@etaderhold etaderhold etaderhold approved these changes

@sureshc sureshc Awaiting requested review from sureshc

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@davidsbailey @etaderhold