Skip to content

Speed up feature_management_spec.rb by removing use of browser#7005

Merged
zachmargolis merged 2 commits intomainfrom
margolis-speed-up-specs
Sep 22, 2022
Merged

Speed up feature_management_spec.rb by removing use of browser#7005
zachmargolis merged 2 commits intomainfrom
margolis-speed-up-specs

Conversation

@zachmargolis
Copy link
Contributor

bundle exec rspec spec/lib/feature_management_spec.rb
# BEFORE
Finished in 2 minutes 38.5 seconds (files took 3.96 seconds to load)
# AFTER
Finished in 0.76372 seconds (files took 7.82 seconds to load)

about a 200x speedup

@zachmargolis zachmargolis requested a review from a team September 21, 2022 22:46
"spec/presenters/setup_presenter_spec.rb": 0.05804999999963911,
"spec/features/remember_device/session_expiration_spec.rb": 5.3716609999974025,
"spec/requests/redirects_spec.rb": 0.12476699999388075,
"spec/lib/feature_management_spec.rb": 158.021550999998,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out how to regenerate the weight for this... so figured it was just easier to remove 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

KNAPSACK_GENERATE_REPORT=true rspec is the way I've done in the past, but that's for all tests, and it takes a very long time to run.

You could generate it for the one file and merge the resulting report into the original file, but I'm not sure if that weight would be accurate on its own without anything to compare it to (unsure if the number is an absolute weight or a relative weight, and unsure if it varies from computer to computer).

KNAPSACK_GENERATE_REPORT=true rspec spec/lib/feature_management_spec.rb

This is what I got when doing that:

Suggested change
"spec/lib/feature_management_spec.rb": 158.021550999998,
"spec/lib/feature_management_spec.rb": 0.27332799999567214,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I typo'd the env variable, but it didn't write that file for me. Anyways this is great, thanks!

And I'm almost certain the values in here are a number of seconds, so they're essentially absolute weights

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I typo'd the env variable, but it didn't write that file for me.

I was having a similar issue, and in my case it was the difference between using KNAPSACK_GENERATE_REPORT=1 and KNAPSACK_GENERATE_REPORT=true. Not sure if that helps in your case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 3a0712b

Copy link
Contributor

Choose a reason for hiding this comment

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

And I'm almost certain the values in here are a number of seconds

Yep, it's seconds

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.

Nice find! Was there a particular way you discovered this, or just stumbled across it? I'm curious now if there are others.

@zachmargolis
Copy link
Contributor Author

zachmargolis commented Sep 22, 2022

Nice find! Was there a particular way you discovered this, or just stumbled across it? I'm curious now if there are others.

I sorted by the values in knapsack report, and looked for things outside of spec/features and this one was a very big surprise

Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

great find! 👏🏼 👏🏼 👏🏼 👏🏼

@zachmargolis zachmargolis merged commit 6f861fe into main Sep 22, 2022
@zachmargolis zachmargolis deleted the margolis-speed-up-specs branch September 22, 2022 15:12
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