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

VERSION 9.0: Fixing rails 3.2 test run #926

Merged
merged 9 commits into from
Sep 10, 2017

Conversation

morozovm
Copy link
Contributor

@morozovm morozovm commented Sep 7, 2017

This change is Reviewable

@morozovm
Copy link
Contributor Author

morozovm commented Sep 7, 2017

hi @justin808 i fixed 3.2 but run into Server Rendering spec failing

Server Rendering
  reloading the server bundle
    reloads the server bundle on a new request if was changed
ActionView::Template::Error (uninitialized constant ReactOnRails::ServerRenderingPool::Exec::Webpacker):
    1: <%= render "header" %>
    2: 
    3: <%= react_component("HelloWorld",
    4:                     props: @app_props_server_render.to_json,
    5:                     prerender: true,
    6:                     trace: true,
  app/views/pages/server_side_hello_world_with_options.html.erb

in log/test.log

is there some ReactOnRails configuration changes missing in dummy_no_webpacker or it is actual issue?

@justin808
Copy link
Member

@morozovm We need to make sure that lines like this:
https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/utils.rb#L77

are never executed if the Webpacker Gem is not installed...

In most cases, we can call using_webpacker? before any call that will use Webpacker methods.

I'm not sure what to do on the rescue statement, other than maybe when the app loads if Webpacker is not defined, then we'll create the Webpacker::Manifest::MissingEntryError. Alternately, we can catch any StandardError and then check if using_webpacker? and the error is the Webpacker::Manifest::MissingEntryError and then do the appropriate thing or else log the error and re-throw.

I think probably the latter is simpler.

@justin808
Copy link
Member

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


rakelib/run_rspec.rake, line 25 at r1 (raw file):

    rspec_args = "spec/react_on_rails --tag ~webpacker --exclude-pattern "\
                 "\"**/generators/*_spec.rb,"\
                 "**/test_helper/webpack_*_spec.rb\""

Nice job on adding the webpacker tag.

CC: @alexfedoseev


spec/react_on_rails/spec_helper.rb, line 30 at r1 (raw file):

require_relative "./simplecov_helper"
# prevent Test::Unit's AutoRunner from executing during RSpec's rake task
Test::Unit.run = true if defined?(Test::Unit) && Test::Unit.respond_to?(:run=)

Why was this happening, @morozovm?


spec/react_on_rails/utils_spec.rb, line 17 at r1 (raw file):

context "With Webpacker enabled and file in manifest", :webpacker => true do·

context "With Webpacker enabled and file in manifest", webpacker: true do

or

context "With Webpacker enabled and file in manifest", :webpacker do

Will either of those work? I typically do the latter.


Comments from Reviewable

@morozovm
Copy link
Contributor Author

morozovm commented Sep 9, 2017

spec/react_on_rails/utils_spec.rb, line 17 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

context "With Webpacker enabled and file in manifest", :webpacker => true do·

context "With Webpacker enabled and file in manifest", webpacker: true do

or

context "With Webpacker enabled and file in manifest", :webpacker do

Will either of those work? I typically do the latter.

fixed


Comments from Reviewable

@morozovm
Copy link
Contributor Author

morozovm commented Sep 9, 2017

@justin808
safe_constantize should do the trick


Comments from Reviewable

@morozovm
Copy link
Contributor Author

morozovm commented Sep 9, 2017

spec/react_on_rails/spec_helper.rb, line 30 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why was this happening, @morozovm?

activesupport 3.2.22.5 demands test-unit to be loaded

begin
  require 'test/unit/testcase'
rescue LoadError => e
  raise LoadError, "Please add test-unit gem to your Gemfile: `gem 'test-unit', '~> 3.0'` (#{e.message})", e.backtrace
end

this hack is solution to loading test-unit and rspec simultaneously by tricking test-unit in into thinking it is already done running. As far as i can see it is only viable solution.


Comments from Reviewable

@morozovm
Copy link
Contributor Author

morozovm commented Sep 9, 2017

rakelib/run_rspec.rake, line 25 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Nice job on adding the webpacker tag.

CC: @alexfedoseev

🙏


Comments from Reviewable

@justin808
Copy link
Member

@morozovm Please rebase your changes on top of master and when github says I can merge, I will.

@justin808
Copy link
Member

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


lib/react_on_rails/utils.rb, line 77 at r2 (raw file):

      @server_bundle_path = begin
        bundle_js_file_path(bundle_name)
      rescue ActiveSupport::Inflector.safe_constantize("Webpacker::Manifest::MissingEntryError")

@morozovm did you try this thoroughly and provide tests for the cases:

  1. exception is thrown
    1a. with Wepbacker
    1b. without Webpacker

I created a small test, and rescue nil seems to crash.

require "active_support/all"

def start_summer
  raise "overheated!"
end

begin
  start_summer
rescue ActiveSupport::Inflector.safe_constantize("Xyz")
  puts "Let me tell you about heat! #{e.inspect}"
end

output:

/Users/justin/.rvm/rubies/ruby-2.4.1/bin/ruby -e $stdout.sync=true;$stderr.sync=true;load($0=ARGV.shift) /Users/justin/Library/Preferences/RubyMine2017.2/scratches/scratch_1.rb
/Users/justin/Library/Preferences/RubyMine2017.2/scratches/scratch_1.rb:9:in `rescue in <top (required)>': class or module required for rescue clause (TypeError)
	from /Users/justin/Library/Preferences/RubyMine2017.2/scratches/scratch_1.rb:7:in `<top (required)>'
	from -e:1:in `load'
	from -e:1:in `<main>'

spec/react_on_rails/spec_helper.rb, line 30 at r1 (raw file):

Previously, morozovm (Maksim Morozov) wrote…

activesupport 3.2.22.5 demands test-unit to be loaded

begin
  require 'test/unit/testcase'
rescue LoadError => e
  raise LoadError, "Please add test-unit gem to your Gemfile: `gem 'test-unit', '~> 3.0'` (#{e.message})", e.backtrace
end

this hack is solution to loading test-unit and rspec simultaneously by tricking test-unit in into thinking it is already done running. As far as i can see it is only viable solution.

what's the resolution of this one?


Comments from Reviewable

@morozovm
Copy link
Contributor Author

lib/react_on_rails/utils.rb, line 77 at r2 (raw file):
@justin808
oops! i guess they changed this behavior long time ago https://bugs.ruby-lang.org/issues/9335

Status changed from Open to Rejected
It had been allowed on 1.9.3 accidentally.
In other words, it's a fixed bug.


Comments from Reviewable

@morozovm
Copy link
Contributor Author

spec/react_on_rails/spec_helper.rb, line 30 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

what's the resolution of this one?

the solution is to set Test::Unit.run = true if Test::Unit defined as i did or i misunderstood the question?


Comments from Reviewable

@morozovm
Copy link
Contributor Author

changes rebased


Comments from Reviewable

@justin808
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@justin808 justin808 merged commit 1a8107e into shakacode:master Sep 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants