Raise error if transaction block is exited in dev/test envs
What does this MR do and why?
This MR turns the following Rails deprecation warning into an error on development/test:
DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block is
deprecated without replacement. If the `throw` came from
`Timeout.timeout(duration)`, pass an exception class as a second
argument so it doesn't use `throw` to abort its block. This results
in the transaction being committed, but in the next release of Rails
It also enables a Rails/TransactionExitStatement
and fixes all offenses (6) and instances not caught by this cop.
The generated TODOs are empty but it's still worth to put this cop in "grace period" to maintain master
stability.
See #410086 (closed).
The fixes
According to https://docs.rubocop.org/rubocop-rails/cops_rails.html#railstransactionexitstatement
As alternatives, it would be more intuitive to explicitly raise an error when rollback is desired, and to use next when commit is desired.
In this MR we use next
instead of break
and return
where appropriate.
How to verify locally
bin/rspec --order defined spec/services/projects/create_service_spec.rb -e "tracks for imported project"
Before
Passes
Projects::CreateService#execute
import data
DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block is
deprecated without replacement. If the `throw` came from
`Timeout.timeout(duration)`, pass an exception class as a second
argument so it doesn't use `throw` to abort its block. This results
in the transaction being committed, but in the next release of Rails
it will rollback.
(called from public_send at /home/peter/devel/gitlab/gdk/gitlab/lib/gitlab/database/load_balancing/connection_proxy.rb:127)
tracks for imported project
After
Fails
Failures:
1) Projects::CreateService#execute import data tracks for imported project
Failure/Error:
expect(Gitlab::Tracking).to have_received(tracking_method) # rubocop:disable RSpec/ExpectGitlabTracking
.with(category, action, **kwargs).at_least(:once)
(Gitlab::Tracking).event("Projects::CreateService", "import_project", {:user=>#<User id:314 @user1>})
expected: at least 1 time with arguments: ("Projects::CreateService", "import_project", {:user=>#<User id:314 @user1>})
received: 0 times
# ./spec/support/helpers/snowplow_helpers.rb:63:in `expect_snowplow_event'
# ./spec/services/projects/create_service_spec.rb:477:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:419:in `block (3 levels) in <top (required)>'
# ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware'
# ./spec/spec_helper.rb:411:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:407:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/application_context.rb:61:in `with_raw_context'
# ./spec/spec_helper.rb:407:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:242:in `block (2 levels) in <top (required)>'
# ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:108:in `block (3 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:62:in `with_cross_joins_prevented'
# ./spec/support/database/prevent_cross_joins.rb:108:in `block (2 levels) in <main>'
Finished in 7.71 seconds (files took 11.1 seconds to load)
1 example, 1 failure
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.