Skip to content

Refactor 2FA setup controller specs#11399

Merged
aduth merged 2 commits intomainfrom
aduth-refactor-2fa-setup-controller-spec
Oct 25, 2024
Merged

Refactor 2FA setup controller specs#11399
aduth merged 2 commits intomainfrom
aduth-refactor-2fa-setup-controller-spec

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 25, 2024

🎫 Ticket

Supports LG-14655

🛠 Summary of changes

Refactors specs for Users::TwoFactorAuthenticationSetupController#create.

Specific improvements:

  • Update describe description to follow conventions for controller action method names ('PATCH create' ➡️ '#create')
  • Remove duplicate assertions for expected analytics logging
  • Avoid testing internal implementation details irrelevant for expected outcomes (asserting that form class is called)
  • Reduce boilerplate, using combination of subject and before common setup
  • Leverage RSpec one-liner syntax to simplify redirect expectations
  • Add test coverage for expected error message on invalid submission

This is split from the work of LG-14655, which requires revisions to these controller specs building upon the improvements here.

📜 Testing Plan

rspec spec/controllers/users/two_factor_authentication_setup_controller_spec.rb

changelog: Internal, Automated Testing, Refactor 2FA setup controller specs
@aduth aduth requested a review from a team October 25, 2024 15:16
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

Looks good and more efficient 👍

@vrajmohan
Copy link
Contributor

Something I never got to the bottom of as some links are busted - https://www.betterspecs.org/#expect.

@aduth
Copy link
Contributor Author

aduth commented Oct 25, 2024

@vrajmohan Yeah it occurred to me after submitting that the one-liner documentation talks about should short-hand using the older syntax. I think we should probably use the is_expected.to syntax, but it also bothers me that it's several more characters in length 😅

Also, in the broader conversation of "should" vs. "expect", I think standardization is much more relevant and impactful when not using the one-liner syntax, since the non-one-liner impacts how the code is read much more than with the one-liner syntax.

expect(some_computed_value).to eq(42)
some_computed_value.should eq(42)

# vs...

it { should eq(42) }
it { is_expected.to eq(42) }

Not really a hill I'd die on either way. 🤷

@aduth
Copy link
Contributor Author

aduth commented Oct 25, 2024

Updated from should to expect syntax in 01b0fde.

@aduth aduth merged commit ad1e9f0 into main Oct 25, 2024
@aduth aduth deleted the aduth-refactor-2fa-setup-controller-spec branch October 25, 2024 19:04
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.

3 participants