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

Class and example reloading #31

Merged
merged 6 commits into from May 30, 2013
Merged

Class and example reloading #31

merged 6 commits into from May 30, 2013

Conversation

@nilbus
Copy link
Collaborator

@nilbus nilbus commented May 30, 2013

This PR contains a few different changes. Please review carefully.

Use Rails 3.2+ class reloading when available

Additional custom_reloaders can also be passed in via options to support
other frameworks like Rails 2.x or custom class reloaders.

I wrote support in Reloaders for prepending, but I didn't hook it up to jruby-rspec. Probably before this is merged, we should decide to either drop prepend support or support a way to prepend reloaders. We might also consider supporting a way to replace the default reloaders. I wanted to get your opinion on this whole Reloaders thing before I fleshed it all out.

This closes #29

Unload previous examples so that they do not accumulate

This fixes #30

Syntax errors now stop the specs from running

Before it would log an error and keep on going anyway, sometimes scrolling an important error off of the screen. The Containment#protect block now throws the symbol that instructs guard that there was an error and to stop testing.

nilbus added 4 commits May 16, 2013
Before it would log an error and keep on going anyway.
The Containment#protect block now throws the symbol that instructs guard
that there was an error and to stop testing.
Additional custom_reloaders can also be passed in via options to support
other frameworks like Rails 2.x or custom class reloaders.

Closes #29
@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

Excellent. Looking at it now.

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

I should note that both the new reload_rails class reloading and the original reload_paths method that uses load are both intended to be run. Originally for Rails projects, I was using the reload_rails method only, but tests started to act very strange and inconsistent after encountering a change that caused a syntax error. I found that loading the class again with load as we had done before eliminated these errors, and still gave us the benefit of letting Rails unload the classes to undefine old methods, etc. I don't believe this causes the classes to be loaded twice, because I think Rails just lazily loads them on demand rather than loading them at reload time.

end

context 'with a custom error_handler' do
subject(:containment) { described_class.new(:error_handler => lambda { @custom_handler_called = true }) }

This comment has been minimized.

@jkutner

jkutner May 30, 2013
Owner

This spec failed for me, with wrong number of arguments (1 for 0). But changing the lambda to lambda { |e| ... fixed it

This comment has been minimized.

@nilbus

nilbus May 30, 2013
Author Collaborator

Ah, I'll add a commit to fix that. I forgot that my JRuby was running in 1.8 mode. Thanks.

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

I'm finding that on subsequent runs, the count of examples is increased. So if the first run is 6 examples, 0 failures, the second is 12 examples, 0 failures, the third is 18 examples, 0 failures and so on. Is that expected?

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

Not at all. This was intended to fix that. Also, was that not happening for you before? Let me see if I can reproduce that.

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

I'm testing it on https://github.com/jkutner/get_back. My Guardfile is this:

interactor :simple
guard "jruby-rspec", :cli => ["-c", "-t~slow"] do 
  watch %r{^spec/.+_spec\.rb$}

It probably was happening with earlier versions and I just never noticed.

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

It's not happening on my 1.8 rails app. I'll try get_back.

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

Ok, I see what happens. When I kick off the tests by pressing Return in the guard console, i get the increased count. But when guard picks up a change to my files, it does not increase.

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

I think we should merge this as is, and address the Return issue in another thread. What do you think?

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

Ah, interesting. I never run the full test suite in guard since it takes about an hour ;-)

This might be a quick fix. I'll take a look before you merge.

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

ok

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

That should do the trick. Now I unload the examples before each run in Runner#run rather than only when files are changed. Good catch!

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

I didn't update the readme documenting the Rails reloading support. One of us should do that before the release, but I won't have time today. Also this should probably be at least a minor version bump (0.2.0?).

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

I'll handle the readme. And I agree that this should be a 0.2.0 release. Awesome work! thanks for the code.

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

I'm getting some pretty odd results when running the specs with your lastest commit. Are you seeing this?

$ bundle exec rspec spec/
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}
.........

Finished in 0.116 seconds
9 examples, 0 failures
RubyArray.java:4020:in `all_pBlockless': java.lang.NullPointerException
    from RubyArray.java:4009:in `all_p'
    from RubyEnumerable.java:1343:in `all_p'
    from RubyEnumerable$INVOKER$s$0$0$all_p.gen:-1:in `call'
    from JavaMethod.java:277:in `call'
        ....
@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

FWIW, the code works on a real project. And running the specs individually works too

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

No, that's pretty strange seeing a NullPointerException since this has no
java extensions. Which JRuby version are you using? It may be a JRuby bug.
I was testing on 1.6.7.2 and didn't see anything like that.

@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

May be the JVM version. I've found a workaround though. Do you see any problems with do this in Guard::JRubyRSpec:

    def run_all
      unload_previous_examples
      super
    end

    def run_on_changes(raw_paths)
      unload_previous_examples
      @reloaders.reload(raw_paths)
@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 30, 2013

I can't think of any other way that specs would run, so I think that should
be fine. Could you apply the change?

@jkutner jkutner merged commit bf676e6 into jkutner:master May 30, 2013
@jkutner
Copy link
Owner

@jkutner jkutner commented May 30, 2013

Merged! Good stuff.

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented May 31, 2013

I'm getting this error now with the latest release. Investigating further.

16:29:43 - ERROR - Guard::JRubyRSpec failed to achieve its <run_on_changes>, exception was:
> [#] NoMethodError: undefined method `match' for #<Array:0x4816505b>
> [#] /home/vagrant/ws/rental_express/ROOT/rails/.bundle/jruby/1.8/gems/guard-rspec-2.5.4/lib/guard/rspec/inspector.rb:50:in `spec_folder?'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/.bundle/jruby/1.8/gems/guard-rspec-2.5.4/lib/guard/rspec/inspector.rb:38:in `should_run_spec_file?'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/.bundle/jruby/1.8/gems/guard-rspec-2.5.4/lib/guard/rspec/inspector.rb:30:in `clean'
> [#] org/jruby/RubyArray.java:2395:in `select'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/.bundle/jruby/1.8/gems/guard-rspec-2.5.4/lib/guard/rspec/inspector.rb:30:in `clean'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/.bundle/jruby/1.8/gems/guard-rspec-2.5.4/lib/guard/rspec/inspector.rb:62:in `clear_spec_files_list_after'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/.bundle/jruby/1.8/gems/guard-rspec-2.5.4/lib/guard/rspec/inspector.rb:29:in `clean'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/.bundle/jruby/1.8/gems/guard-rspec-2.5.4/lib/guard/rspec.rb:85:in `run_on_changes'
> [#] /vagrant/git/guard-jruby-rspec/lib/guard/jruby-rspec.rb:74:in `run_on_changes'
> [#] org/jruby/RubyKernel.java:2080:in `send'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:99:in `run_supervised_task'
> [#] org/jruby/RubyKernel.java:1183:in `catch'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:97:in `run_supervised_task'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:144:in `run_first_task_found'
> [#] org/jruby/RubyArray.java:1615:in `each'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:142:in `run_first_task_found'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:79:in `run_on_changes'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:175:in `scoped_guards'
> [#] org/jruby/RubyArray.java:1615:in `each'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:173:in `scoped_guards'
> [#] org/jruby/RubyKernel.java:1183:in `catch'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:172:in `scoped_guards'
> [#] org/jruby/RubyArray.java:1615:in `each'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:170:in `scoped_guards'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard/runner.rb:72:in `run_on_changes'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard.rb:105:in `setup_listener'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard.rb:368:in `within_preserved_state'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard.rb:365:in `within_preserved_state'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/guard-1.8.0/lib/guard.rb:104:in `setup_listener'
> [#] org/jruby/RubyProc.java:270:in `call'
> [#] org/jruby/RubyProc.java:220:in `call'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/listen-1.0.2/lib/listen/listener.rb:234:in `on_change'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/listen-1.0.2/lib/listen/listener.rb:266:in `initialize_adapter'
> [#] org/jruby/RubyProc.java:270:in `call'
> [#] org/jruby/RubyProc.java:220:in `call'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/listen-1.0.2/lib/listen/adapter.rb:239:in `report_changes'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/listen-1.0.2/lib/listen/adapter.rb:308:in `poll_changed_directories'
> [#] /home/vagrant/ws/rental_express/ROOT/rails/vendor/gems/listen-1.0.2/lib/listen/adapter.rb:284:in `start_poller'
> [#] org/jruby/RubyProc.java:270:in `call'
> [#] org/jruby/RubyProc.java:224:in `call'
16:29:43 - INFO - Guard::JRubyRSpec has just been fired
@jkutner
Copy link
Owner

@jkutner jkutner commented May 31, 2013

Are you seeing that with the 0.2.0 gem and not the code in master?

@ruprict
Copy link
Contributor

@ruprict ruprict commented May 31, 2013

I am seeing this in master. It seems to stem from the controller watch in the Guardfile:

watch(%r{^app/controllers/(.+)_(controller)\.rb$})  { |m| ["spec/routing/#{m[1]}_routing_spec.rb", "spec/#{m[2]}s/#{m[1]}_#{m[2]}_spec.rb", "spec/acceptance/#{m[1]}_spec.rb"] }

As the error goes awayf for me if I just map that watch to one of those values in the array. Will try to dig deeper in a bit.

@nilbus
Copy link
Collaborator Author

@nilbus nilbus commented Jun 1, 2013

Opening a new issue #34

@nilbus nilbus deleted the nilbus:class-reloading branch Jun 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.