[WIP] Don't merge, testing 5.1

[jrafanie]

No description provided.

[jrafanie]

@h-kataria @himdel can you help on the rails 5.1 asset deprecations on this PR? This is the same error I was seeing on UI-classic and it happens because we disable live compilation of assets in test:

DEPRECATION WARNING: The asset "layout/login-screen-logo.png" is not present in the asset pipeline.Falling back to an asset that may be in the public folder.
This behavior is deprecated and will be removed.
To bypass the asset pipeline and preserve this behavior,
use the `skip_pipeline: true` option.
 (called from image_path at /home/travis/build/jrafanie/manageiq-api/app/controllers/api/api_controller.rb:89)

Basically, because we disable compilation, the asset can't be found in the pipeline in the test environment. If I allow compilation in test.rb, the deprecation goes away:

index c41a2eb9b2..dd254fda87 100644
--- a/config/environments/test.rb
+++ b/config/environments/test.rb
@@ -23,7 +23,7 @@ Vmdb::Application.configure do
   end

   config.assets.debug = true
-  config.assets.compile = %w(spec:javascript spec).include?(ENV['TEST_SUITE'])
+  config.assets.compile = true

I feel like we should enable compilation, the default behavior in test... Are you opposed to doing this?

[himdel]

The main reason to disable compilation was that it was saving 3 minutes per test run. (ManageIQ/manageiq#18228)

If that issue has not gone away, maybe we should try keeping it false...

Are we talking about just that one file?
Because if so, maybe something like

--- a/app/views/dashboard/login.html.haml
+++ b/app/views/dashboard/login.html.haml
@@ -1,7 +1,7 @@
 - auth_mode = ::Settings.authentication.mode
 
 %span#badge
-  %img{:src => ::Settings.server.custom_logo ? '/upload/custom_logo.png' : image_path("layout/login-screen-logo.png")}
+  %img{:src => ::Settings.server.custom_logo ? '/upload/custom_logo.png' : image_path("layout/login-screen-logo.png", :skip_pipeline => ENV['TEST_SUITE'] == 'spec:javascript')}
 
 .container
   .row

would do the trick?

(Or, possibly we could

if ENV['TEST_SUITE'] == "spec:javascript"
  helper do
    def image_path(path, options = {})
      path
    end
  end
end

in ApplicationController (well, a mixin/helper/something)?)

[jrafanie]

The main reason to disable compilation was that it was saving 3 minutes per test run. (ManageIQ/manageiq#18228)

If that issue has not gone away, maybe we should try keeping it false...

How do I verify this is still a problem when running a test locally? Does it hang at some point that's predictable and observable?

Are we talking about just that one file?

No. There were many. I was looking at a larger solution as it's the same problem I was hitting with ui-classic. The workaround I had in ui-classic was to force any tests on travis or locally to precompile assets and in the case of travis still run yarn. I don't have the exact numbers but I know there were tens of places that ui-classsic + api tests had this complaint. I don't really like the need to compile assets.

I can try adding in-line skip_pipeline.

I'll also try the monkey patch of image_path.

Why isn't this a problem for core rails apps? Whenever we change default behaviors, it feels like there's other options we can choose as this sometimes bites us.

[himdel]

How do I verify this is still a problem when running a test locally? Does it hang at some point that's predictable and observable?

The first test after 'render_view' that calls asset_path or its children
methods like image_path or stylesheet_path are called triggers asset
compilation and therefor lasts for 1.5-3m depending on CPU.

(from the original PR)

Why isn't this a problem for core rails apps?

No idea, sorry, maybe they have less assets? Or they always run assets:precompile?
I'm guessing spending minutes in asset compilation is the unusual part ;)

[himdel]

Cc @martinpovolny in case you remember a specific test..

[himdel]

@jrafanie Thinking a bit more, maybe the precompilation time will go down once Patternfly is no longer going through the asset pipeline (ManageIQ/manageiq-ui-classic#5679).

That should cut the time down to seconds, and at that point we can probably merrily compile :).

(But, that's just a theory at this point, fonts and images are not yet resolved in the PR.)

[jrafanie]

@himdel I tried a thing...

ManageIQ/manageiq@e2dc951

Let's see if it works... Why does sprockets-rails need to stat_tree all the assets to determine if it's precompiled in test mode? If it's not there, we want to live compile it, right?

Here's the partial flamegraph:

That was from running bundle exec rspec ./spec/views/layouts/listnav/_ems_cloud.html.haml_spec.rb locally... which was taking 2 minutes...like you said...

Before:

05:20:18 ~/Code/manageiq-ui-classic (rails-5-1) (2.5.3) - bundle exec rspec ./spec/views/layouts/listnav/_ems_cloud.html.haml_spec.rb
....
Finished in 2 minutes 53.7 seconds (files took 14.26 seconds to load)
4 examples, 0 failures

After:

05:39:09 ~/Code/manageiq-ui-classic (rails-5-1) (2.5.3) + bundle exec rspec ./spec/views/layouts/listnav/_ems_cloud.html.haml_spec.rb
....

Finished in 7.06 seconds (files took 11.58 seconds to load)
4 examples, 0 failures

[himdel]

Ooh, that looks great, thanks! :)

Does it affect what asset_path actually returns, or just the time?

[jrafanie]

@himdel I don't know. I can try it later. Do we have tests that use asset_path I could try? I see only 1 test failure on ui-classic using this technique: https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/544451084

Do you understand why the sprockets-rails code was stat'ing all of the asset locations at rails engine boot? ManageIQ/manageiq@e2dc951

While I like the results, I don't know if my monkey patch is doing the right thing. I don't understand all of the complexity of asset compilation vs. pre-compilation in test vs. dev vs prod. It feels wrong to build a list of all asset paths throughout the app in test mode at engine load time if we're going to dynamically compile things anyway.

[himdel]

I think we mostly try not to have them, as long as no new tests are breaking that's probably fine.
(The one failure you have is unrelated, core strikes again ManageIQ/manageiq-ui-classic#4921 (comment) .)

Do you understand why the sprockets-rails code was stat'ing all of the asset locations at rails engine boot?

Well, heh heh, nobody undestands sprockets :)
But if I had to guess, it's because it's running in the non-development mode of serving only known assets, so it has to build up the list. If the list is there, it's presumably checking whether all the files still exist or something like that.
And this being the asset pipeline with multiple roots to search in... well, there are reasons we're dropping it :).

So.. I'm really treating sprockets as a legacy thing that will go away.
From that point of view, any solution that doesn't break anything is probably fine :).

[miq-bot]

Checked commit jrafanie@6fb09fd with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

[bdunne]

@jrafanie Anything remaining to be done with this?

[jrafanie]

@jrafanie Anything remaining to be done with this?

Nope, thanks!