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

Preload assets before backend specs #2117

Merged

Conversation

jhawthorn
Copy link
Contributor

We've been seeing some builds fail with "Request failed to reach server" on the first request.

This happens because we have quite a few assets and it can take some time to compile them. Since this was done on the first request, this sometimes caused poltergeist to error.

This commit gets sprockets-rails to preload the assets before the suite is run (if there is a feature spec included in the run).

This has the added advantage of not including the time to precompile specs as part of the first spec's run time. Running rspec --profile=10 should now be more useful in showing actually slow specs.

Alternative to #2116

We've been seeing some builds fail with "Request failed to reach server"
on the first request.

This happens because we have quite a few assets and it can take some
time to compile them. Since this was done on the first request,
this sometimes caused poltergeist to error.

This commit gets sprockets-rails to preload the assets before the suite
is run (if there is a feature spec included in the run).

This has the added advantage of not including the time to precompile
specs as part of the first spec's runtime. Running rspec --profile=10
should now be more useful in showing actually slow specs.
Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

Looks like a nice solution

@jhawthorn
Copy link
Contributor Author

jhawthorn commented Jul 25, 2017

Before

$ rspec --profile=10 ./spec/features/admin/reports_spec.rb

Randomized with seed 54973
..

Top 2 slowest examples (15.09 seconds, 99.5% of total time):
  Reports visiting the admin reports page should have the right content
    14.61 seconds ./spec/features/admin/reports_spec.rb:7
  Reports searching the admin reports page should allow me to search for reports
    0.48311 seconds ./spec/features/admin/reports_spec.rb:49

Finished in 15.17 seconds (files took 2.36 seconds to load)
2 examples, 0 failures

Randomized with seed 54973

After

$ rspec --profile=10 ./spec/features/admin/reports_spec.rb --seed=54973

Randomized with seed 54973
..

Top 2 slowest examples (0.89705 seconds, 5.9% of total time):
  Reports searching the admin reports page should allow me to search for reports
    0.57514 seconds ./spec/features/admin/reports_spec.rb:49
  Reports visiting the admin reports page should have the right content
    0.32191 seconds ./spec/features/admin/reports_spec.rb:7

Finished in 15.27 seconds (files took 2.35 seconds to load)
2 examples, 0 failures

Randomized with seed 54973

Time for the suite hasn't changed (the ~14 seconds to load assets is just moved earlier), but the first specs run in a more similar time to the other specs in the suite and won't error waiting on assets to compile.

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

This seems great!

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

👌🏻

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

🥇

@mtomov
Copy link
Contributor

mtomov commented Jul 26, 2017

I will happily steal this right off the bat into our projects, as I've struggled with this for quite a while now with no good solution on the horizon. Thanks a lot!

@jhawthorn jhawthorn merged commit e791835 into solidusio:master Jul 26, 2017
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.

6 participants