Skip to content

Loading…

Report actual exceptions when requires fail #126

Merged
merged 3 commits into from

2 participants

@richmeyers

Swallowing exceptions makes for some misleading problem reports. For example, when broach is installed on a system that does not use rubygems and one attempts to run tests, the result is:

% rake test:acceptance
(in /home/test1/integrity)
/usr/local/lib/ruby/vendor_ruby/1.8/dm-core.rb:26: warning: already initialized constant Mash
DataMapper::Sweatshop::Unique - ParseTree could not be loaded, anonymous uniques will not be allowed
Loaded suite /usr/local/lib/ruby/vendor_ruby/1.8/rake/rake_test_loader
Started
EEE...Install broach to use the Campfire notifier
rake aborted!
Command failed with status (1): [/usr/local/bin/ruby -I"lib:test" "/usr/loc...]

(See full trace by running task with --trace)

With attached patch applied:

% rake test:acceptance                 
(in /home/test1/integrity)
/usr/local/lib/ruby/vendor_ruby/1.8/dm-core.rb:26: warning: already initialized constant Mash
DataMapper::Sweatshop::Unique - ParseTree could not be loaded, anonymous uniques will not be allowed
Loaded suite /usr/local/lib/ruby/vendor_ruby/1.8/rake/rake_test_loader
Started
EEE...Install broach to use the Campfire notifier: LoadError: no such file to load -- rubygems
rake aborted!
Command failed with status (1): [/usr/local/bin/ruby -I"lib:test" "/usr/loc...]

(See full trace by running task with --trace)

True reason for failure is Manfred/broach#6.

@richmeyers

Found another similar case in the delayed job test, where activerecord requires sqlite3.

Notifo requires httparty which requires rubygems.

Why not simply let these exceptions propagate instead of hiding the stack trace? What I do now is:

  1. Run rake test
  2. It fails with a message saying "install foo"
  3. I launch irb and require 'foo'
  4. I look at the stack trace to see what actually required whatever it is that I don't have.

In case of activerecord -> sqlite3 dependency even this did not work as delayed job could be required in irb without errors. I had to reraise the error in the test to see what caused it.

@richmeyers

The last commit converts abort calls to warn + raise, yielding two benefits:

  1. Exception stack trace is preserved.
  2. The tests now proceed all the way when one of them depends on something that is not installed.
@grahamc grahamc closed this
@grahamc grahamc merged commit 0f3a37d into integrity:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
5 lib/integrity/notifier/campfire.rb
@@ -1,7 +1,8 @@
begin
require "broach"
-rescue LoadError
- abort "Install broach to use the Campfire notifier"
+rescue LoadError => e
+ warn "Install broach to use the Campfire notifier: #{e.class}: #{e.message}"
+ raise
end
module Integrity
View
5 lib/integrity/notifier/email.rb
@@ -1,7 +1,8 @@
begin
require "pony"
-rescue LoadError
- abort "Install pony to use the Email notifier"
+rescue LoadError => e
+ warn "Install pony to use the Email notifier: #{e.class}: #{e.message}"
+ raise
end
module Integrity
View
5 lib/integrity/notifier/notifo.rb
@@ -1,7 +1,8 @@
begin
require "notifo"
-rescue LoadError
- abort "Install notifo to use the Notifo notifier"
+rescue LoadError => e
+ warn "Install notifo to use the Notifo notifier: #{e.class}: #{e.message}"
+ raise
end
module Integrity
View
5 test/acceptance/manual_build_test.rb
@@ -180,8 +180,9 @@ def build
click_link "my-test-project"
assert_have_tag("h1", :content => "Built #{repo.short_head} successfully")
- rescue LoadError, NameError
- warn "Couldn't load DJ. Skipping test"
+ rescue LoadError, NameError => e
+ warn "Couldn't load DJ. Skipping test: #{e.class}: #{e.message}"
+ raise
ensure
# TODO
Integrity.config.instance_variable_set(:@builder, old_builder)
Something went wrong with that request. Please try again.