Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minitest rails not working on 9000 #2491

Closed
lephyrius opened this issue Jan 21, 2015 · 12 comments
Closed

Minitest rails not working on 9000 #2491

lephyrius opened this issue Jan 21, 2015 · 12 comments
Assignees

Comments

@lephyrius
Copy link

I use minitest-rails as a testsuit for my rails app but cannot get it working on 9000.
So when I run it under 1.7.18 it executes all my controller tests with:
rake test:controllers

Under jRuby 9000 it only runs:
** Invoke test:controllers (first_time)
** Invoke test:prepare (first_time)
** Execute test:prepare
** Execute test:controllers
Then just quits no output at all.

@headius
Copy link
Member

headius commented Jan 22, 2015

Interesting...sounds like it's bailing out when running some subprocesses.

Can you make a little dummy app for us to reproduce this? I'm not sure how to set it up myself.

@headius headius added this to the 9.0.0.0.pre2 milestone Jan 22, 2015
@headius headius self-assigned this Jan 22, 2015
@lephyrius
Copy link
Author

@headius I tried to create an dummy app...
Just a note it doesn't reach "setup" or any methods in my tests. Also it doesn't react differently on "rake test" or "rake test:models".
I need to poke around more.
Is there a good/simple way of seeing all the ruby methods called when I run "rake test:controllers"? Then I could figure out where it quits.

@lephyrius
Copy link
Author

I have now found the error I will put up a test app as soon as I have cleaned it up a bit. Also I found a bonus error which I also include that I found in my desperate testing.(that you will see when you run the tests on 9000 but not on 1.7.18 or MRI 2.2) I will put it on github. :)

@lephyrius
Copy link
Author

I created the dummy app and it's available here:
https://github.com/lephyrius/minitest-9000-test
I tried to make it as minimal as possible. But rails is rails and they have a lot of cruft.
To reproduce just bundle and run "rake test:controllers"

@subbuss
Copy link
Contributor

subbuss commented Jan 24, 2015

Hmm .. I couldn't reproduce it with the test app and latest master.

[subbu@earth minitest-9000-test] jruby -S rake test:controllers
[subbu@earth minitest-9000-test] jruby -S rake --verbose test:controllers
[subbu@earth minitest-9000-test] jruby -v
jruby 9.0.0.0-SNAPSHOT (2.2.0p0) 2015-01-23 0aef21d Java HotSpot(TM) 64-Bit Server VM 24.76-b04 on 1.7.0_76-b13 +jit [linux-amd64]

@lephyrius
Copy link
Author

@subbuss Hmmmm...
So ruby test:controllers doesn't output anything? Then u have found the bug.
If I run "rake test:controllers" under "ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-darwin14]":
I get this message:
rake test:controllers
Run options: --seed 1890

Running:

..

Finished in 0.047701s, 41.9278 runs/s, 41.9278 assertions/s.

2 runs, 2 assertions, 0 failures, 0 errors, 0 skips

When I run it under 9000:
ruby -v
jruby 9.0.0.0-SNAPSHOT (2.2.0p0) 2015-01-24 bbb7bbe Java HotSpot(TM) 64-Bit Server VM 25.0-b70 on 1.8.0-b132 +jit [darwin-x86_64]
I get no output at all...

@enebo
Copy link
Member

enebo commented Jan 24, 2015

I have seen an issue when working on test_refinements that IR bugs throwing exceptions can at time cause the execution of the class body to abort but not actually raise and abort execution. We definitely should get some magic catchall (which I thought the C2 crasher was about) to see this condition. I am not clear on how it happens.

@subbuss
Copy link
Contributor

subbuss commented Feb 11, 2015

Phew .. after digging around a bit, here is what I found.

@enebo @headius So, if you instrument the at_exit method in minitest-5.5.1/lib/minitest.rb .. in IR mode, the test run exits with: "in autorun with method 'method_missing' not defined in FactoryGirl::DefinitionProxy" whereas in jruby 1.7, no such exception is raised.

To be continued ...

@subbuss
Copy link
Contributor

subbuss commented Feb 11, 2015

That might be a bogus exception, or it might be that $! has not been cleared .. but, some progress at least. This can be debugged by instrumenting where $! is being set / cleared in the JRuby runtime.

@subbuss
Copy link
Contributor

subbuss commented Feb 14, 2015

So, I dug into this just now and quickly found that this is basically a dupe of #1601 which is still open. So, there are several places in the runtime where RaiseException is being caught but $! is not being saved/restored properly. In this case, a failure in RubyModule undefMethod sets $! but since it overwrites the old value (rather than save-restore as should happen), this dirtied value of $! propagates all the way to the end and causes this failure as above.

@subbuss
Copy link
Contributor

subbuss commented Feb 14, 2015

I can now confirm that with this diff below, minitest app runs to completion. Rather than do a one-off commit (which I am happy to do), will wait to chat with @enebo and @headius to see how to handle #1601 .. why the new IR runtime is requiring this change and why the old AST runtime didn't require this. I am fairly positive that the new runtime solution is the right fix, but maybe I am missing something ...

diff --git a/core/src/main/java/org/jruby/RubyModule.java b/core/src/main/java/org/jruby/RubyModule.java
index a53603c..b6e47fa 100644
--- a/core/src/main/java/org/jruby/RubyModule.java
+++ b/core/src/main/java/org/jruby/RubyModule.java
@@ -860,9 +860,11 @@ public class RubyModule extends RubyObject {

         if (name.equals("method_missing")) {

+            IRubyObject oldExc = runtime.getGlobalVariables().get("$!");
             try {
                 removeMethod(context, name);
             } catch (RaiseException t) {
+                runtime.getGlobalVariables().set("$!", oldExc);
                 if(!(t.getException() instanceof RubyNameError)) {
                     throw t;
                 }

subbuss added a commit that referenced this issue Feb 14, 2015
* Whenever a RaiseException is caught and handled, the $! should
  be the restored to the previous value of the exception before
  the try-catch was entered.

* This fixes #1601 and #2491 but additional code auditing and
  discussion will be helpful.
@subbuss
Copy link
Contributor

subbuss commented Feb 14, 2015

I've fixed all instances I could find that were missing $! save restore. This should fix this specific failure for minitest. Please reopen if it is not fixed. Let us continue the other part of the discussion on #1601 ..

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

No branches or pull requests

4 participants