jejacks0n / teaspoon

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

Re-add support for Sprockets 4 #478

Closed liseki closed 8 years ago

liseki commented 8 years ago

@jejacks0n Sorry about the previous attempt modeset/teaspoon#473. I was quiet for a while but I have it all working now. The appraisal tests are all good and I have also tested this out in a Rails 4.2.5/Sprockets 4 project. This however depends on rails/sprockets#341 so we should wait on that before merging. After we merge I can work on some changes to the README including the version of sprockets-rails needed (version 3 plus that patch).

liseki commented 8 years ago

A few more important points:

jejacks0n commented 8 years ago

I find this new behavior of Sprockets to be confusing, but I'm willing to roll with this and see.

jejacks0n commented 8 years ago

At some point when sprockets changes slow back down, I'd like to lock it to a minimum version and remove the hacks we have in place for various versions.

liseki commented 8 years ago

Sounds good. Dropping support for Sprockets 2 will give room for cleaning up.

jejacks0n commented 8 years ago

Thanks @liseki. Sounds like this should wait until sprockets is ready? If so, do you want me to wait until you check back in?

liseki commented 8 years ago

@jejacks0n yes please. I'll wait for the sprockets-rails patch to go through and then I'll rebase and squash these commits and give you the heads up.

jejacks0n commented 8 years ago

I appreciate all your work on this. 👍

liseki commented 8 years ago

OK @jejacks0n, let's see if we can close this today!

As I was developing this patch I was also testing it out on this setup:

rails 4.2.5
sprockets 3.6.0
sprockets-rails 3.0.4

I kept thinking I was breaking Teaspoon in this setup when config.expand_assets = false but as it turns out Teaspoon is broken. I suspect this has not been a problem because config.expand_assets = true is the default and there is not much reason for users changing this. The problem with this setup is that ActionView::Helpers::AssetTagHelper#path_to_javascript is always passed the option debug = true from Teaspoon. This causes sprockets-rails to append the body query param which makes Sprockets serve a 'self' asset when we actually want the debug (bundled) one. Again part of the confusion here is the difference in meaning of a 'debug' asset between Sprockets 2 and 4 (with Sprockets 3 being somewhere in between). I am providing the explanation just for posterity because I decided to leave this as is; as long no one is having a problem with it then it isn't a problem :)

Having that out of the way, I simplified by using the _allow_nonprecomiled option which allows us the luxury of dropping the manifest file and the sprockets-rails patch. As with the previous attempt, we load "debug" assets by default and "self" assets when running coverage tests. All the appraisal tests are happy and I have tested this with both config.expand_assets = true and config.expand_assets = false in the following setups:

Setup 1:
rails 3.2.22
sprockets 2.2.3

Setup 2:
rails 4.2.5
sprockets 3.6.0
sprockets-rails 3.0.4

Setup 3:
rails 4.2.5
sprockets 4.0.0.beta2
sprockets-rails 3.0.4

As mentioned above, Setup 2 still fails when config.expand_assets = false. Switching to rails 4.2.3 in Setup 2 still fails when config.expand_assets = false since we loose our debug/allow_non_precompiled option and now need to make sure all our assets are declared in precompile list.

Let me know if you are happy with this and I'll squash for committing. Sorry that you'll end up with two consecutive git history entries on Sprockets 4 but I think now we can finally settle it.

jejacks0n commented 8 years ago

No worries, go ahead and squash. There's always been a bit of an issue with the expand_assets configuration, but I couldn't get both scenarios working well initially, and haven't had the opportunity to go back and see if there's a better option.. Generally speaking any failed test/stack traces are much more useful when you DO expand assets, but for performance reasons some people want to avoid expanding their assets. My point being that it will probably need additional attention at some point.

liseki commented 8 years ago

Since I have most of this fresh in my mind I'll take a look at getting that setup to work towards the end of the week. I think part of the issue is the conflation of the asset helper options "debug" and "allow_non_precompiled" within ActionView.

As far as Sprocket 4 is concerned, this is a go. Thank you for this awesome tool!

liseki commented 8 years ago

Sorry, that conflation happens within sprockets-rails not ActionView.

jejacks0n commented 8 years ago

I was super confused by the hack for allow_non_precompiled and how that makes it through the chain (or didn't as far as I could tell).. e.g. the debug option does several different things, and isn't consistent.

jejacks0n commented 8 years ago

Status check @liseki.. Feel free to disregard if nothing has changed.

liseki commented 8 years ago

This is good to go! :)

liseki commented 8 years ago

@jejacks0n I think this is safe to merge unless you have some other thoughts. Since the rails 4 + sprocket 3 issue precedes this change then we need not wait on fixing it. As far as Sprockets 4 goes, there is nothing else to add here.

jejacks0n commented 8 years ago

Thanks for your efforts here. Much appreciated.

liseki commented 8 years ago

It's a pleasure to help on this awesome tool!