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

Pre-compile all needed assets in CI environment #12552

Closed
wants to merge 2 commits into from

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jun 5, 2024

What? Why?

We use Webpacker and Sprockets to compile assets. I hadn't realised
before that we were still compiling lots of JS with Sprockets. So even
after we pre-compiled assets with Webpacker, we still needed to compile
more assets on the first page visit in test environment. That made some
time-critical specs, like caching specs, fail.

This approach pre-compiles all assets but only in CI. Running the
compilation task adds 6.5 seconds even if there's nothing to compile
while the previous Webpacker call returned instantly. So I decided to
run the compilation only on CI and activated on-demand compilation to
the test environment.

Time-critical specs will still be flaky without pre-compilation locally
but I figured that a fair trade-off for faster specs in general. During
most development, we don't need all assets to be compiled and it's
faster to just compile what's needed. And running a time-critical spec
twice solves the flakiness.

Do you agree with this approach? Do you have a better idea?

What should we test?

  • Specs only.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@mkllnk mkllnk self-assigned this Jun 5, 2024
@mkllnk mkllnk added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jun 5, 2024
mkllnk added 2 commits June 6, 2024 10:20
We use Webpacker *and* Sprockets to compile assets. I hadn't realised
before that we were still compiling lots of JS with Sprockets. So even
after we pre-compiled assets with Webpacker, we still needed to compile
more assets on the first page visit in test environment. That made some
time-critical specs, like caching specs, fail.

This approach pre-compiles all assets but only in CI. Running the
compilation task adds 6.5 seconds even if there's nothing to compile
while the previous Webpacker call returned instantly. So I decided to
run the compilation only on CI and activated on-demand compilation to
the test environment.

Time-critical specs will still be flaky without pre-compilation locally
but I figured that a fair trade-off for faster specs in general. During
most development, we don't need all assets to be compiled and it's
faster to just compile what's needed. And running a time-critical spec
twice solves the flakiness.
I'm not sure why, but the pre-compiling of assets triggered Rails to
render `style="..."` instead of `style='...'` in this case. But when
assets are compiled on-demand, we get the single quotes. So I changed
the spec to be agnostic of this detail. We actually just want to know
about the link and its href.
@mkllnk mkllnk force-pushed the precompile-assets branch from 9150ae4 to 3a389ba Compare June 6, 2024 00:27
@mkllnk mkllnk changed the title Pre-compile all needed assets in test environment Pre-compile all needed assets in CI environment Jun 6, 2024
@mkllnk mkllnk marked this pull request as ready for review June 6, 2024 00:42
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Good find !

@dacook
Copy link
Member

dacook commented Jun 13, 2024

Sorry I didn't get to have a good look at this yet. I don't like the idea of adding 6.5sec to most CI runs, but I'm not sure there's any better solutions. Maybe if we can isolate those specs that require sprockets we can run it only where needed. Will have a look next week.

@dacook dacook self-requested a review June 13, 2024 07:48
@dacook
Copy link
Member

dacook commented Jun 18, 2024

Given that this adds time to compile all assets, which might not be used, I had a go at Cache public assets in CI.
But I'm not sure it will work. @mkllnk do you might taking a look to see if it makes sense to you?

@mkllnk
Copy link
Member Author

mkllnk commented Jun 18, 2024

Oh, I had not realised that this is doubling CI run time, from 6 minutes to 12 minutes. That's not great, of course. The easiest quick fix in this case would be to put a retry: 2 on the flaky spec. So in the second run the assets are compiled and the test should pass.

@dacook
Copy link
Member

dacook commented Jun 18, 2024

this is doubling CI run time

Hmm that's weird, but certainly not good!

Ideally, I'd like us to be able to cache the necessary parts to speed up the asset compilation, AND pre-compile as per this PR, because that replicates a prod instance.

But it doesn't seem practical, so retry is an appropriate alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Flaky] rspec ./spec/system/consumer/caching/shops_caching_spec.rb:17
3 participants