jejacks0n / teaspoon

Teaspoon: Javascript test runner for Rails. Use Selenium, BrowserStack, or PhantomJS.
1.43k stars 244 forks source link

Failure running tests with rails 4 and sprockets 3 when config.expand_assets = false #482

Closed liseki closed 3 years ago

liseki commented 8 years ago

Running tests with the following setup:

rails 4.2.3
sprockets 3.6.0
sprockets-rails 3.0.4

and config.expand_assets = false raises a Sprockets::Rails::Helper::AssetNotPrecompiled error. This is because this version of rails does not take advantage of Teaspoon's patch of ActionView::Helpers::AssetTagHelper#javascript_include_tag.

Switching to rails 4.2.5 does not raise an error however tests may fail because sprockets gives us a 'self' asset version of spec_helper.js which strips out any sprockets directives. See this comment for an explanation of why this happens.

liseki commented 8 years ago

The simplest way to solve this (and allow other gems to easily require assets not listed for pre-compilation) is to make sprockets-rails and action view respect both the 'debug' and 'allow_non_precompiled' flags to asset tag helpers. Once that happens we won't need to make any changes to Teaspoon, and we can even do away with Teaspoon's patch for later versions.

As a first step I have submitted rails/sprockets-rails#347. I will update here as progress is made.

liseki commented 8 years ago

@jejacks0n Having those sprockets-rails and action view changes would allow us to close rails/sprockets-rails#297.

liseki commented 8 years ago

With the advent of sprockets-rails commit b4f9006c we have something to work with: it provides the global config option config.assets.check_precompiled_asset which defaults to true.

To firstly solve these Rails 4 issues we have to remove the javascript_include_tag override. When config.expand_assets = false this override still sets the 'debug' option to true and as I explained previously this makes sprockets return 'self' assets when we actually want the concatenated ones.

Secondly, we need to tell sprockets not the throw exceptions about missing assets. We have two choices:

  1. Add a notice to the documentation that the developer should set config.assets.check_precompiled_asset = false. Developers will probably run into exceptions at some point if they have not set this option (and have config.expand_assets = false). This is a bit of an unpleasant developer experience.
  2. Set config.assets.check_precompiled_asset = false within Teaspoon and have everything just work for the developer. While this is a nice developer experience it is a bit sneaky that we have set a global option for the developer's app and they may well want and expect exceptions to be thrown when assets are called that are not declared for precompilation.

It is a bit of a shame that rails/sprockets-rails#347 did not take because having a granular option would have allowed Teaspoon to solve this issue without changing the developer's app configuration. I have left my thoughts there for consideration.

So what do think @jejacks0n? How would you like to move forward with this?

bouk commented 8 years ago

The best fix for the pipeline stuff is maybe just to do something different depending on the sprockets version. I have a hacky patch here: https://github.com/bouk/teaspoon/commit/9dab441203c5dcbf63bc203920dfa7ba1833d90c that only sets the pipeline to debug if the sprockets version is >= 4 and this fixes the issue of sprockets directives not working when expand_assets = false on master

liseki commented 8 years ago

@bouk what versions of sprockets and sprockets-rails are you running? And did you remember to clear your asset cache before trying the patch? I have tried it out with sprockets 3.6.0 and master sprockets-rails, and the issue is still the same with both rails 4.2.3 and 4.2.5.

mathieujobin commented 3 years ago

this is solved now