Use last version of sprockets-rails and sync tests for assets. #7482

Merged
merged 2 commits into from Sep 19, 2012

3 participants

@route

For now tests are a bit outdated and we're using sprockets-rails from rubygems, I think to use it from github sounds more reasonable.

/cc @guilleiguaran

BTW: We should add changelog entry about removing manifest option, I think.

@guilleiguaran
Ruby on Rails member

@route please don't remove sprockets-rails from rails.gemspec, adding it to Gemfile should be enough to use it in rails tests.

btw, all tests related to assets are passing after of apply this?

@route

Thank you, I'll revert rails.gemspec, sorry of course it should be there. Yes all tests pass. I'll push it in a few minutes.

@route

@guilleiguaran just ping!

@rafaelfranca
Ruby on Rails member

@guilleiguaran just ping us when this one is ready to merge

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 8, 2012
railties/test/application/assets_test.rb
quietly do
- precompile_task = 'bundle exec rake assets:precompile'
- precompile_task += ' ' + env if env
- out = Dir.chdir(app_path){ %x[ #{precompile_task} ] }
- assert $?.exitstatus == 0,
@rafaelfranca
Ruby on Rails member

Are you sure that we can remove this one?

@guilleiguaran
Ruby on Rails member

This was done in ebb906d, then rails updated and sprockets-rails is outdated, this need to be done in sprockets-rails

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 8, 2012
railties/test/application/assets_test.rb
quietly do
- precompile_task = 'bundle exec rake assets:precompile'
- precompile_task += ' ' + env if env
- out = Dir.chdir(app_path){ %x[ #{precompile_task} ] }
- assert $?.exitstatus == 0,
- "#{precompile_task} has failed: #{out}.\
- Probably you didn't install JavaScript runtime."
- return out
- end
- end
-
- def clean_assets!
@rafaelfranca
Ruby on Rails member

Why you removed this one?

@guilleiguaran
Ruby on Rails member

Same here, also done in ebb906d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Sep 8, 2012
railties/test/application/assets_test.rb
end
- def assert_file_exists(filename)
@rafaelfranca
Ruby on Rails member

Why remove this?

@guilleiguaran
Ruby on Rails member

This is fine and should be done in sprockets-rails, see 481dac9 for reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilleiguaran guilleiguaran commented on the diff Sep 9, 2012
railties/test/application/assets_test.rb
@@ -154,21 +141,6 @@ def assert_no_file_exists(filename)
assert_match(/application-([0-z]+)\.css/, assets["application.css"])
end
- test "precompile creates a manifest file in a custom path with all the assets listed" do
@guilleiguaran
Ruby on Rails member

I'm surprised, this is passing in Rails master? Probably the last version of sprockets-rails isn't being used

@guilleiguaran
Ruby on Rails member

Ah, I see, we aren't using sprockets-rails from git in Rails, so the change in Gemfile in this PR is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilleiguaran guilleiguaran commented on an outdated diff Sep 9, 2012
railties/test/application/assets_test.rb
@@ -241,7 +213,7 @@ def show_detailed_exceptions?() true end
get '/posts'
assert_match(/AssetNotPrecompiledError/, last_response.body)
- assert_match(/app.js isn't precompiled/, last_response.body)
+ assert_match(/app\.js isn.+t precompiled/, last_response.body)
@guilleiguaran
Ruby on Rails member

The assertion in rails master looks better, please send a PR to sprockets-rails updating this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilleiguaran guilleiguaran commented on the diff Sep 9, 2012
@@ -27,7 +27,8 @@ end
# This needs to be with require false to avoid
# it being automatically loaded by sprockets
-gem 'uglifier', '>= 1.0.3', require: false
+gem 'uglifier', require: false
+gem 'sprockets-rails', github: 'rails/sprockets-rails'
@guilleiguaran
Ruby on Rails member

I'm getting a failure in rails test suite using sprockets-rails from rails repo, can you verify if you are getting that error also?

@route
route added a note Sep 9, 2012

Urgh, I cannot track it down, my tests are green, and I wonder why?

@guilleiguaran
Ruby on Rails member

can you test running tests with https://github.com/guilleiguaran/rails, in the remove-manifest-option branch?
I think is a problem with my machine

@route
route added a note Sep 10, 2012

Yes I have the same issue as you. Trying to find out.

@route
route added a note Sep 10, 2012

This

      images_should_compile = ["a.png", "happyface.png", "happy_face.png", "happy.face.png",
                               "happy-face.png", "happy.happy_face.png", "happy_happy.face.png",
                               "happy.happy.face.png", "happy", "happy.face", "-happyface",
                               "-happy.png", "-happy.face.png", "_happyface", "_happy.face.png",
                               "_happy.png"]

should be replaced with

      images_should_compile = ["a.png", "happyface.png", "happy_face.png", "happy.face.png",
                               "happy-face.png", "happy.happy_face.png", "happy_happy.face.png",
                               "happy.happy.face.png", "-happy.png", "-happy.face.png",
                               "_happy.face.png", "_happy.png"]

Rake was aborted because of -happyface has no extension. I think it because of old frozen versions of sprockets-rails and sprockets, isn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilleiguaran
Ruby on Rails member

@route @rafaelfranca btw, can you check my comment in ebb906d?

@rafaelfranca
Ruby on Rails member

I think we should find a better way that assert the exit status

@route

Updated, we should make decision about exit status, and I wonder why @guilleiguaran has a failure, but I don't.

@route

I think it's ready to merge now /cc @guilleiguaran

@route

@guilleiguaran any chance to review it?

@guilleiguaran
Ruby on Rails member

@route it's ready for me

@rafaelfranca can you merge this?

@rafaelfranca rafaelfranca merged commit bf4c8fe into rails:master Sep 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment