Skip to content

Update rubocop-rails and other testing/development gems#8961

Merged
mitchellhenke merged 6 commits intomainfrom
mitchellhenke/update-test-deps
Aug 9, 2023
Merged

Update rubocop-rails and other testing/development gems#8961
mitchellhenke merged 6 commits intomainfrom
mitchellhenke/update-test-deps

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Aug 8, 2023

🛠 Summary of changes

In working on #8932, I realized that we haven't updated our test/dev dependencies in a while. rubocop-rails is the most significant one, but I've updated other ones as well.

One of the new checks that I did not enable, but fixed a few of the errors is Rails/ActionControllerFlashBeforeRender as it had a few false positives because it can't handle conditionals across methods. This is one of the situations where it sees a call to flash without a redirect, but it's fine because it is located elsewhere. We could think about enabling it, but it would mean ignoring in a handful of places and/or doing some refactoring.

Copy link
Contributor Author

@mitchellhenke mitchellhenke Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because of this change in webmock

Without it, it will fail to parse the URL and raise an error like the following:

expected UspsInPersonProofing::Exception::RequestEnrollException, got #
<RSpec::Mocks::MockExpectationError:"#<ActiveSupport::Logger:0x0000000125656e60 @level=0, 
@progname=..."Exception raised while enrolling user:
Absolute URI missing hierarchical segment: 'http://:80'\")"> with backtrace:

Comment on lines -10 to -15
Copy link
Contributor Author

@mitchellhenke mitchellhenke Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of Rails 7, returning in a transaction will rollback, which is not the intended behavior in this method, though it does not affect it since there is no prior change we'd want applied.

The new Rails/TransactionExitStatement caught it. A test was added below to check that it works correctly.

Mitchell Henke added 6 commits August 8, 2023 17:41
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/update-test-deps branch from 07346b0 to 1f13487 Compare August 8, 2023 22:42
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks for the explainer comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants