bundle exec rake assets:precompile shouldn't fail quietly. #6627

Merged
merged 1 commit into from Jun 5, 2012

Conversation

Projects
None yet
5 participants
Contributor

Vanuan commented Jun 4, 2012

If JavaScript runtime is not installed, execjs fails with error quietly,
while tests continue to run. This should not happen since it causes tests
to fail for unknown reason (#6621).

This commit assures that if JavaScript runtime is not installed, an assertion
is raised.

Member

arunagw commented Jun 5, 2012

@josevalim josevalim and 1 other commented on an outdated diff Jun 5, 2012

railties/test/application/assets_test.rb
quietly do
- Dir.chdir(app_path){ `bundle exec rake assets:precompile` }
+ 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 nodejs"
@josevalim

josevalim Jun 5, 2012

Member

I don't think we should hardcode to nodejs since many are available. Wouldn't out be enough to tell us what happened? Also why is execjs/sprockets writing to stdout instead of stderr? :( Thanks for looking into this!

@Vanuan

Vanuan Jun 5, 2012

Contributor

I don't think we should hardcode to nodejs since many are available.

Ok, I'll replace "nodejs" with "JavaScript runtime".

Wouldn't out be enough to tell us what happened?
Also why is execjs/sprockets writing to stdout instead of stderr?

I'm not sure about this. Why do you think execjs writes to stdout?

@Vanuan

Vanuan Jun 5, 2012

Contributor

The assets:precompile task writes to stderr on failure, so out won't tell us what happened:

bundle exec rake assets:precompile >/dev/null
/usr/share/ruby-rvm/rubies/ruby-1.9.3-p194/bin/ruby /usr/share/ruby-rvm/gems/ruby-1.9.3-p194/bin/rake assets:precompile:all RAILS_ENV=production RAILS_GROUPS=assets
rake aborted!
Could not find a JavaScript runtime. See https://github.com/sstephenson/execjs for a list of available runtimes.
  (in /home/user/projects/rails/railties/tmp/app/app/assets/javascripts/application.js)
@Vanuan Vanuan bundle exec rake assets:precompile shouldn't fail quietly.
If JavaScript runtime is not installed, execjs fails with error quietly,
while tests continue to run. This should not happen since it causes tests
to fail for unknown reason (#6621).

This commit assures that if JavaScript runtime is not installed, an assertion
is raised.
ebb906d
Contributor

Vanuan commented Jun 5, 2012

I've updated the commit.

josevalim merged commit 9b742fd into rails:master Jun 5, 2012

Owner

guilleiguaran commented on ebb906d Sep 9, 2012

This is hiding and making hard to track some errors, for example changing to rails to use last sprockets-rails version, with this commit, I'm getting as error:

  1) Failure:
test_0004_precompile application.js and application.css and all other non JS/CSS files(ApplicationTests::AssetsTest) [test/application/assets_test.rb:25]:
bundle exec rake assets:precompile has failed: .                Probably you didn't install JavaScript runtime.

reverting this commit I get a better idea about what is happening:

 1) Failure:
test_0004_precompile application.js and application.css and all other non JS/CSS files(ApplicationTests::AssetsTest) [test/application/assets_test.rb:97]:
missing /private/var/folders/nn/w852mb3j0xldmfmk32sgqz380000gn/T/d20120909-79528-s81jv8/app/public/assets/a.png
Contributor

route replied Sep 9, 2012

Off the top of my head, since assets compilation runs in the separate process and should always be successful we can capture STDERR and instead of probably error we can show to user original. The original issue #6621 is really hard to track in tests if we won't catch exit status.

Contributor

Vanuan replied Sep 10, 2012

This commit doesn't hide anything. It just shows that precompile step has failed and quits earlier. It shows the exact line of failure. If you revert it you wouldn't know what is the cause in most cases. The missing file doesn't instantly make you aware that the precompile task has failed.

I agree that "Probably you didn't install JavaScript runtime" message might be misleading, but the "bundle exec rake assets:precompile has failed" part is perfectly correct.

I also agree that stderr should've been catched instead. I think it would be something like:

err = Dir.chdir(app_path){  %x[ #{precompile_task} 2>\&1 ] }
assert $?.exitstatus == 0,  "#{precompile_task} has failed: #{err}."
Contributor

Vanuan replied Sep 10, 2012

Oh, I see somebody has already implemented this: #7586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment