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

Spec speed ups on AccountsController spec #25391

Merged

Conversation

mjankowski
Copy link
Contributor

As noted towards the end of #25369 (comment) - when I was working on the various Sidekiq/Fabricate/Paperclip spec speedup PRs, another pattern I noticed was lots of spec files where we have a (moderately expensive) setup step happening in a before block which is getting run for every example in it's context.

This is sometimes fine if the setup is trivial and the examples are complex and would become severely unreadable by combining them, but in other areas there's an expensive setup and the examples are all checking against the same result ... and because of the before running for each one it's expensive to do.

Before doing a widescale change like this on the whole suite, I wanted to open this as an example of what I'm thinking. I tried to pick a spec which was repetitive enough that I guessed there might be speedups, but simple enough that it wouldn't be overwhelming to change/review.

The diff here is pretty noisy and it will almost definitely be easier to understand the changes by stepping through one commit at a time - I think they are pretty atomic/descriptive on their own.

All that said, the changes here:

  • There was a block of checks within each format (rss, json, html) which didn't need any of the status records to exist because they are purely checking on responses based on the account status. I put a context around the status creation, and pulled those checks out into a different one with just the account but not the statuses.
  • Many of the checks within the RSS context were just asserting the present of tag manager URLs from the response body ... but each example was repeating the whole request. I lumped these into single examples per context and tried to extract/refactor a but on method naming so its clear what the assertions are.
  • Magnitude not as large as RSS area, but also combined some checks in the html/json response contexts.

Before changes this spec was taking ~23 seconds to run locally. After changes it takes ~11s locally. I have no idea if this magnitude of change is higher/lower/normal etc for this one file vs the whole rest of the spec.

Would love feedback here before I do more of this on other specs.

@renchap renchap added testing Automated lint and test suites ruby Pull requests that update Ruby code labels Jun 13, 2023
@mjankowski mjankowski force-pushed the spec-performance-controller-accounts branch from 8e42f70 to 77327c2 Compare June 13, 2023 13:08
@mjankowski mjankowski changed the title Spec speed ups on accounts controller spec Spec speed ups on AccountsController spec Jun 22, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@ClearlyClaire ClearlyClaire merged commit 12bb7be into mastodon:main Oct 17, 2023
34 checks passed
@mjankowski mjankowski deleted the spec-performance-controller-accounts branch October 17, 2023 12:58
audiodude pushed a commit to audiodude/mastodon that referenced this pull request Oct 23, 2023
000hen pushed a commit to thenapnetwork/nap-mastodon that referenced this pull request Dec 6, 2023
vmstan pushed a commit to vmstan/mastodon that referenced this pull request Dec 14, 2023
vmstan pushed a commit to vmstan/mastodon that referenced this pull request Jan 5, 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.

None yet

3 participants