Skip to content
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.

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

Convert admin/settings controller specs to system specs #31548

Conversation

mjankowski
Copy link
Contributor

Related to ongoing discussion on #30195

This is sort of a minimal proof of concept of starting to migrate some of these, but also is viable to merge. Before I do a bigger set, let alone all/most of them, I want to make sure we're on the same page style-wise, wording-wise, etc - in regard to converting any of these.

For this one specifically:

  • With the exception of the branding controller spec here, these were all very very minimal specs with barely any assertions - which is part of why I picked this as a starting point. All the conversions have the same exact coverage (hitting the show view, filling in a form, submitting form, asserting something about results) which is technically a coverage increase (none of the controller specs have any assertions about results beyond http response code). This could be further enhanced with some assertions about db/settings changes, but I wanted to keep it close to a 1:1 conversion for this first pass.
  • This is a very slight LOC reduction from the controller to system specs (offset a bit by adding the helper in this PR), and preserves the 100% coverage that the controller classes had before.
  • I anticipate having better abstractions and more helpers as we evolve this to more places (probably some helper gems as well for some of the capybara interactions?) but again, wanted to start simple here and not pre-abstract too much. I did a first pass with all of the specs very verbose/repetitive, and started to pull some stuff out, but then stopped myself before I went too far.
  • There's a slight execution speed-up from creating fewer factories post-conversion, but also a small slowdown from exercising the full server/browser interaction ... more or less a wash there, and hard to measure given small size of things; but we may want to keep an eye on this as we convert more.

@mjankowski mjankowski force-pushed the convert-controller-to-system-specs-admin-settings branch from c87fa47 to 1767070 Compare August 23, 2024 12:33
@mjankowski mjankowski force-pushed the convert-controller-to-system-specs-admin-settings branch 2 times, most recently from dae212c to 7faec18 Compare August 26, 2024 15:46
@mjankowski mjankowski force-pushed the convert-controller-to-system-specs-admin-settings branch from 7faec18 to fea8a6f Compare August 28, 2024 13:52
@mjankowski mjankowski added testing Automated lint and test suites ruby Pull requests that update Ruby code labels Aug 28, 2024
@Gargron Gargron added this pull request to the merge queue Sep 3, 2024
Merged via the queue into mastodon:main with commit 928390c Sep 3, 2024
35 checks passed
@mjankowski mjankowski deleted the convert-controller-to-system-specs-admin-settings branch September 3, 2024 15:49
justinwritescode pushed a commit to justinwritescode/mastodon that referenced this pull request Sep 15, 2024
nileane pushed a commit to nileane/mastodon that referenced this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ruby Pull requests that update Ruby code testing Automated lint and test suites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants