JRuby

assert_raise fails on native exceptions

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: JRuby 1.5.6
  • Fix Version/s: JRuby 1.7.0.pre1
  • Component/s: None
  • Labels:
    None
  • Number of attachments :
    1

Description

assert_raise in test/unit does not properly handle NativeExceptions. When given one to test against, the code in _check_exception_class checks if the class inherits from the ruby Exception class (with `assert(Exception >= klass,...`). This fails when a NativeException is passed into it. A similar bug was made earlier as JTESTR-45, but the reporter focused on rspec (which was found to be invalid) and did not follow through with test/unit and the bug was not elevated to JRUBY. Also, this seems to have been known on the web at http://stackoverflow.com/questions/2187166/jruby-and-testunits-assert-raise with no resolution.

This code reproduces the problem:

require 'test/unit'
require 'java'

class TestAssertRaiseWithNativeException < Test::Unit::TestCase
  java_import('java.util.NoSuchElementException') { 'FooError' }
  java_import 'java.util.MissingResourceException'
  def test_native_exception_with_renaming
    assert_raise(FooError) {
      raise FooError
    }
  end

  def test_native_exception
    assert_raise(Java::JavaUtil::MissingResourceException) {
      raise Java::JavaUtil::MissingResourceException
    }
  end
end

The above tests will fail with these results:

  1) Failure:
test_native_exception(TestAssertRaiseAliasedNativeException) [assert_raise_problem.rb:15]:
Should expect a class of exception, Java::JavaUtil::MissingResourceException.
<nil> is not true.

  2) Failure:
test_native_exception_with_renaming(TestAssertRaiseAliasedNativeException) [assert_raise_problem.rb:8]:
Should expect a class of exception, Java::JavaUtil::NoSuchElementException.
<nil> is not true.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jeff Hodges added a comment -

Noticed that this test case will not pass properly because the `raise` keyword in jruby does not handle native exception classes.

Changing `raise FooError` to `raise FooError.new` and `raise Java::JavaUtil::MissingResourceException` to `raise Java::JavaUtil::MissingResourceException.new('f','f','f')` will have tests that will correctly pass when _check_exception_class is patched.

Show
Jeff Hodges added a comment - Noticed that this test case will not pass properly because the `raise` keyword in jruby does not handle native exception classes. Changing `raise FooError` to `raise FooError.new` and `raise Java::JavaUtil::MissingResourceException` to `raise Java::JavaUtil::MissingResourceException.new('f','f','f')` will have tests that will correctly pass when _check_exception_class is patched.
Hide
Permalink
Jeff Hodges added a comment -

This is a patch with tests that corrects this ticket.

It adds the test file test/test_assert_raise_with_native_exception.rb to the end of jruby_index. This may not be the correct place for it.

This patch is also available as the branch assert_raise_native_exceptions on my (jmhodges) fork of jruby: https://github.com/jmhodges/jruby/tree/assert_raise_native_exceptions.

This would be lovely to get in the next dot release. I'm taking matters into my own hands and monkey-patching this in my own tests.

Show
Jeff Hodges added a comment - This is a patch with tests that corrects this ticket. It adds the test file test/test_assert_raise_with_native_exception.rb to the end of jruby_index. This may not be the correct place for it. This patch is also available as the branch assert_raise_native_exceptions on my (jmhodges) fork of jruby: https://github.com/jmhodges/jruby/tree/assert_raise_native_exceptions . This would be lovely to get in the next dot release. I'm taking matters into my own hands and monkey-patching this in my own tests.
Hide
Permalink
Jeff Hodges added a comment - - edited

Note: this does not seem to fix a problem where if the native exception is raised from inside a java code path, assert_raise fails by not correctly detecting the class of the error. A NativeException is raised, and the assert_raise code could check its `cause` method for the actual raised error. This is probably worthy of another bug, but I'm not sure how to implement the test in the suite as it would require new native code to be written and put somewhere in the tests CLASSPATH.

Also, it's late and I'm lazy.

Show
Jeff Hodges added a comment - - edited Note: this does not seem to fix a problem where if the native exception is raised from inside a java code path, assert_raise fails by not correctly detecting the class of the error. A NativeException is raised, and the assert_raise code could check its `cause` method for the actual raised error. This is probably worthy of another bug, but I'm not sure how to implement the test in the suite as it would require new native code to be written and put somewhere in the tests CLASSPATH. Also, it's late and I'm lazy.
Hide
Permalink
Charles Oliver Nutter added a comment -

There's a bit of a transitional conundrum here. The original creators of JRuby decided that calls to Java that raise exceptions should instead raise NativeException that wraps it. More recently, we have learned how to make Ruby actually handle real Java exceptions, for cases where we don't catch it and wrap it with NativeException. So now there's two possible ways a Java exception might manifest in a Ruby trace.

For the case you're talking about, the exceptions are almost always NativeException, and assert_raise(NativeException) will work properly. For cases where the exception is caused not by a Ruby-to-Java call but by some part of JRuby or some library JRuby calls internally, you would need to assert_raise(TheJavaException). I believe things are behaving as expected here; you just need to know whether to expect the NativeException or not.

At the moment, the only solution I have to merge these two worlds without drastically changing how people handle Java exceptions from Ruby would be to make NativeException into a module, mix it into java.lang.Throwable, and allow it to be the target of a rescue (which may already work). This would allow us to start propagating Java exceptions as-is (without wrapper) without breaking existing code that expects to rescue NativeException. It would also cause your expectations to work, since instead of propagating the wrapped object we'd actually be propagating the real exception...

Thoughts?

Show
Charles Oliver Nutter added a comment - There's a bit of a transitional conundrum here. The original creators of JRuby decided that calls to Java that raise exceptions should instead raise NativeException that wraps it. More recently, we have learned how to make Ruby actually handle real Java exceptions, for cases where we don't catch it and wrap it with NativeException. So now there's two possible ways a Java exception might manifest in a Ruby trace. For the case you're talking about, the exceptions are almost always NativeException, and assert_raise(NativeException) will work properly. For cases where the exception is caused not by a Ruby-to-Java call but by some part of JRuby or some library JRuby calls internally, you would need to assert_raise(TheJavaException). I believe things are behaving as expected here; you just need to know whether to expect the NativeException or not. At the moment, the only solution I have to merge these two worlds without drastically changing how people handle Java exceptions from Ruby would be to make NativeException into a module, mix it into java.lang.Throwable, and allow it to be the target of a rescue (which may already work). This would allow us to start propagating Java exceptions as-is (without wrapper) without breaking existing code that expects to rescue NativeException. It would also cause your expectations to work, since instead of propagating the wrapped object we'd actually be propagating the real exception... Thoughts?
Hide
Permalink
Jeff Hodges added a comment -

Mixing in java.lang.Throwable to NativeException definitely seems like the correct approach to maintain API compatibility in the right places. Yadda yadda, hopes for future API breakage to clean that up, yadda yadda.

Sounds good to me.

Show
Jeff Hodges added a comment - Mixing in java.lang.Throwable to NativeException definitely seems like the correct approach to maintain API compatibility in the right places. Yadda yadda, hopes for future API breakage to clean that up, yadda yadda. Sounds good to me.
Hide
Permalink
Charles Oliver Nutter added a comment -

JRuby 1.7 makes Java exceptions raise as themselves, rescuable as Object, Exception, NativeException or any of their Java supertypes.

Show
Charles Oliver Nutter added a comment - JRuby 1.7 makes Java exceptions raise as themselves, rescuable as Object, Exception, NativeException or any of their Java supertypes.

People

  • Assignee:
    Charles Oliver Nutter
    Reporter:
    Jeff Hodges
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: