From 377d1e86c60b59ec9e7fa5e0f73fe343fe8247b9 Mon Sep 17 00:00:00 2001 From: Alec Hipshear Date: Tue, 5 Aug 2014 15:15:32 -0400 Subject: [PATCH 1/7] [#75] Fix a timing bug with poltergeist/turbolinks There's a timing bug in ViewHelperTest that randomly results in ```ReferenceError: Turbolinks Not Found```. This causes unrelated builds to fail, which gums up the whole open source works. Per https://github.com/teampoltergeist/poltergeist#timing-problems, the suggested way of fixing this is to just add calls to sleep. If you wish to test this in the future, use the shell code: ```for i in `seq 1 10`; do rake appraisal; done``` This should give you enough test runs to hit the timing bug at least once. --- test/test_helper.rb | 4 ++++ test/view_helper_test.rb | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/test/test_helper.rb b/test/test_helper.rb index 77725df8c..125699b37 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -22,3 +22,7 @@ if ActiveSupport::TestCase.method_defined?(:fixture_path=) ActiveSupport::TestCase.fixture_path = File.expand_path("../fixtures", __FILE__) end + +def wait_for_turbolinks_to_be_available + sleep(1) +end diff --git a/test/view_helper_test.rb b/test/view_helper_test.rb index e80796de0..81c74b0ff 100644 --- a/test/view_helper_test.rb +++ b/test/view_helper_test.rb @@ -62,10 +62,14 @@ class ViewHelperTest < ActionDispatch::IntegrationTest page.execute_script('history.back();') assert page.has_content?('Hello Alice') + wait_for_turbolinks_to_be_available() + # Try Turbolinks javascript API. page.execute_script('Turbolinks.visit("/pages/2");') assert page.has_content?('Hello Alice') + wait_for_turbolinks_to_be_available() + page.execute_script('Turbolinks.visit("/pages/1");') assert page.has_content?('Hello Bob') From 93c0529f7ee470f09fc0644429c333d7aa35c40f Mon Sep 17 00:00:00 2001 From: Alec Hipshear Date: Sat, 5 Jul 2014 17:31:04 -0400 Subject: [PATCH 2/7] Fix asset reloading in development mode The renderer was using a memoized class variable to cache the concatinated react.js and components.js for server-side rendering. This meant that when you updated a component, the server-rendered version would not be refreshed. Since #setup! is called every time a watched file is updated, it seems like the natural place to reset the memozied string. --- lib/react/rails/railtie.rb | 10 +++--- lib/react/renderer.rb | 1 + test/helper_files/TodoListWithUpdates.js.jsx | 22 +++++++++++++ test/server_rendered_html_test.rb | 33 ++++++++++++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-) create mode 100644 test/helper_files/TodoListWithUpdates.js.jsx create mode 100644 test/server_rendered_html_test.rb diff --git a/lib/react/rails/railtie.rb b/lib/react/rails/railtie.rb index b567a9f67..13c858933 100644 --- a/lib/react/rails/railtie.rb +++ b/lib/react/rails/railtie.rb @@ -56,13 +56,15 @@ class Railtie < ::Rails::Railtie config.after_initialize do |app| # Server Rendering # Concat component_filenames together for server rendering - app.config.react.components_js = app.config.react.component_filenames.map do |filename| - app.assets[filename].to_s - end.join(";") + app.config.react.components_js = lambda { + app.config.react.component_filenames.map do |filename| + app.assets[filename].to_s + end.join(";") + } do_setup = lambda do cfg = app.config.react - React::Renderer.setup!( cfg.react_js.call, cfg.components_js, + React::Renderer.setup!( cfg.react_js.call, cfg.components_js.call, {:size => cfg.size, :timeout => cfg.timeout}) end diff --git a/lib/react/renderer.rb b/lib/react/renderer.rb index 521415484..be17bc614 100644 --- a/lib/react/renderer.rb +++ b/lib/react/renderer.rb @@ -10,6 +10,7 @@ def self.setup!(react_js, components_js, args={}) @@react_js = react_js @@components_js = components_js @@pool.shutdown{} if @@pool + @@combined_js = nil if defined? @@combined_js @@pool = ConnectionPool.new(:size => args[:size]||10, :timeout => args[:timeout]||20) { self.new } end diff --git a/test/helper_files/TodoListWithUpdates.js.jsx b/test/helper_files/TodoListWithUpdates.js.jsx new file mode 100644 index 000000000..b3950c95b --- /dev/null +++ b/test/helper_files/TodoListWithUpdates.js.jsx @@ -0,0 +1,22 @@ +/** @jsx React.DOM */ + +TodoList = React.createClass({ + getInitialState: function() { + return({mounted: "nope"}); + }, + componentWillMount: function() { + this.setState({mounted: 'yep'}); + }, + render: function() { + return ( + + ) + } +}) + diff --git a/test/server_rendered_html_test.rb b/test/server_rendered_html_test.rb new file mode 100644 index 000000000..c6810bda7 --- /dev/null +++ b/test/server_rendered_html_test.rb @@ -0,0 +1,33 @@ +require 'test_helper' +require 'fileutils' + +class ServerRenderedHtmlTest < ActionDispatch::IntegrationTest + test 'react server rendering reloads jsx after changes to the jsx files' do + file_with_updates = File.expand_path('../helper_files/TodoListWithUpdates.js.jsx', __FILE__) + file_without_updates = File.expand_path('../helper_files/TodoListWithoutUpdates.js.jsx', __FILE__) + app_file = File.expand_path('../dummy/app/assets/javascripts/components/TodoList.js.jsx', __FILE__) + + sleep(1) + FileUtils.cp app_file, file_without_updates + # fixes inconsistent test runs; phantomjs doesn't always see that the file was + # updated without manually calling #touch on it + FileUtils.touch app_file + + begin + get '/server/1' + refute_match 'Updated', response.body + + sleep(1) + FileUtils.cp file_with_updates, app_file + FileUtils.touch app_file + + get '/server/1' + assert_match 'Updated', response.body + ensure + # if we have a test failure, we want to make sure that we revert the dummy file + sleep(1) + FileUtils.mv file_without_updates, app_file + FileUtils.touch app_file + end + end +end From 75a3188495a1602558d13b6c751cf3d194a48f73 Mon Sep 17 00:00:00 2001 From: Alec Hipshear Date: Sun, 13 Jul 2014 11:32:01 -0400 Subject: [PATCH 3/7] Kludge to fix Tubrolinks intermittent test errors Sometimes Turbolinks hasn't fully loaded before Capybara attempts to call 'Turbolinks.visit'. Adding a brief sleep gives Turbolinks a chance to become availabe to PhantomJS --- test/view_helper_test.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/view_helper_test.rb b/test/view_helper_test.rb index e80796de0..1a2a09aee 100644 --- a/test/view_helper_test.rb +++ b/test/view_helper_test.rb @@ -66,6 +66,10 @@ class ViewHelperTest < ActionDispatch::IntegrationTest page.execute_script('Turbolinks.visit("/pages/2");') assert page.has_content?('Hello Alice') + # Sometimes Turbolinks isn't available right away. We need to wait for a brief moment + # for phantomJS to finish loading javascript. + sleep(0.1) + page.execute_script('Turbolinks.visit("/pages/1");') assert page.has_content?('Hello Bob') From e9b6697b22c025300358d6c6c207eed9f72cdc4a Mon Sep 17 00:00:00 2001 From: Alec Hipshear Date: Sun, 13 Jul 2014 14:04:22 -0400 Subject: [PATCH 4/7] Fixes a gnarly test bug with server side rendering Background: Running tests multiple times in a row could result in errors, and could result in complete passing. In other words, the order the tests ran in determined whether or not the tests would pass. Ruby Test::Unit uses a psuedorandom seed to randomize the order that tests are run in; you can specify a certain seed with TESTOPS="--seed=xyz", which fixes the seed and thus the order the tests will be run in. The main source of the bug was that many of the tests rely on replacing asset files and asserting that the results are as expected; however, the asset pipeline uses mtime to determine when files have been changed. *mtime's resolution is only 1 second, meaning that when a series of actions took less than a second to run, the mtime wouldn't actually change, and Rails asset pipeline wouldn't update the file list.* This commit adds a hook to force reset the internal concatinated react.js and components.js string. Another test requires a few calls to sleep(1), because it is specifically testing that the asset pipeline picks up changes to the file on disk correctly and renders them server side. --- lib/react/rails/railtie.rb | 2 +- lib/react/renderer.rb | 18 +++++++++++++----- test/react_test.rb | 2 ++ test/server_rendered_html_test.rb | 14 +++++++++----- 4 files changed, 25 insertions(+), 11 deletions(-) diff --git a/lib/react/rails/railtie.rb b/lib/react/rails/railtie.rb index 13c858933..3a10b1c85 100644 --- a/lib/react/rails/railtie.rb +++ b/lib/react/rails/railtie.rb @@ -64,7 +64,7 @@ class Railtie < ::Rails::Railtie do_setup = lambda do cfg = app.config.react - React::Renderer.setup!( cfg.react_js.call, cfg.components_js.call, + React::Renderer.setup!( cfg.react_js, cfg.components_js, {:size => cfg.size, :timeout => cfg.timeout}) end diff --git a/lib/react/renderer.rb b/lib/react/renderer.rb index be17bc614..bc60c0868 100644 --- a/lib/react/renderer.rb +++ b/lib/react/renderer.rb @@ -10,7 +10,7 @@ def self.setup!(react_js, components_js, args={}) @@react_js = react_js @@components_js = components_js @@pool.shutdown{} if @@pool - @@combined_js = nil if defined? @@combined_js + reset_combined_js! @@pool = ConnectionPool.new(:size => args[:size]||10, :timeout => args[:timeout]||20) { self.new } end @@ -20,8 +20,8 @@ def self.render(component, args={}) end end - def self.combined_js - @@combined_js ||= <<-CODE + def self.setup_combined_js + <<-CODE var global = global || this; var console = global.console || {}; @@ -31,12 +31,20 @@ def self.combined_js } }); - #{@@react_js}; + #{@@react_js.call}; React = global.React; - #{@@components_js}; + #{@@components_js.call}; CODE end + def self.reset_combined_js! + @@combined_js = setup_combined_js + end + + def self.combined_js + @@combined_js + end + def context @context ||= ExecJS.compile(self.class.combined_js) end diff --git a/test/react_test.rb b/test/react_test.rb index c78243e69..81166ab0e 100644 --- a/test/react_test.rb +++ b/test/react_test.rb @@ -31,6 +31,8 @@ class ReactTest < ActionDispatch::IntegrationTest assert_response :success assert_equal "'test_confirmation_token_react_content';\n", @response.body + + React::Renderer.reset_combined_js! end end diff --git a/test/server_rendered_html_test.rb b/test/server_rendered_html_test.rb index c6810bda7..67ee758ff 100644 --- a/test/server_rendered_html_test.rb +++ b/test/server_rendered_html_test.rb @@ -2,22 +2,26 @@ require 'fileutils' class ServerRenderedHtmlTest < ActionDispatch::IntegrationTest + # Rails' asset pipeline has trouble picking up changes to files if they happen too fast. + # By sleeping for a little bit at certain points, we can make sure that rails notices the + # change in the file mtime, and calls our renderer setup functions appropriately + def wait_to_ensure_asset_pipeline_detects_changes + sleep(1) + end + test 'react server rendering reloads jsx after changes to the jsx files' do file_with_updates = File.expand_path('../helper_files/TodoListWithUpdates.js.jsx', __FILE__) file_without_updates = File.expand_path('../helper_files/TodoListWithoutUpdates.js.jsx', __FILE__) app_file = File.expand_path('../dummy/app/assets/javascripts/components/TodoList.js.jsx', __FILE__) - sleep(1) FileUtils.cp app_file, file_without_updates - # fixes inconsistent test runs; phantomjs doesn't always see that the file was - # updated without manually calling #touch on it FileUtils.touch app_file begin get '/server/1' refute_match 'Updated', response.body - sleep(1) + wait_to_ensure_asset_pipeline_detects_changes FileUtils.cp file_with_updates, app_file FileUtils.touch app_file @@ -25,7 +29,7 @@ class ServerRenderedHtmlTest < ActionDispatch::IntegrationTest assert_match 'Updated', response.body ensure # if we have a test failure, we want to make sure that we revert the dummy file - sleep(1) + wait_to_ensure_asset_pipeline_detects_changes FileUtils.mv file_without_updates, app_file FileUtils.touch app_file end From 352c491241d347bb2a1bb193475e8c6fcd96fb96 Mon Sep 17 00:00:00 2001 From: Alec Hipshear Date: Sun, 13 Jul 2014 15:33:43 -0400 Subject: [PATCH 5/7] Can't reliably reproduce this bug Since I can't reliably reproduce the ```Turbolinks Not Found``` error, this code is basically just hopeful thinking. --- test/view_helper_test.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/view_helper_test.rb b/test/view_helper_test.rb index 1a2a09aee..e80796de0 100644 --- a/test/view_helper_test.rb +++ b/test/view_helper_test.rb @@ -66,10 +66,6 @@ class ViewHelperTest < ActionDispatch::IntegrationTest page.execute_script('Turbolinks.visit("/pages/2");') assert page.has_content?('Hello Alice') - # Sometimes Turbolinks isn't available right away. We need to wait for a brief moment - # for phantomJS to finish loading javascript. - sleep(0.1) - page.execute_script('Turbolinks.visit("/pages/1");') assert page.has_content?('Hello Bob') From 1f7906a4ad1821ef32efeaa20cdbcbc9f0b50fcb Mon Sep 17 00:00:00 2001 From: Alec Hipshear Date: Sun, 13 Jul 2014 16:22:38 -0400 Subject: [PATCH 6/7] Revert "Can't reliably reproduce this bug" This reverts commit 55d2a1d6aaffeb5f796989382a6ec7e825737436. --- test/view_helper_test.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/view_helper_test.rb b/test/view_helper_test.rb index e80796de0..1a2a09aee 100644 --- a/test/view_helper_test.rb +++ b/test/view_helper_test.rb @@ -66,6 +66,10 @@ class ViewHelperTest < ActionDispatch::IntegrationTest page.execute_script('Turbolinks.visit("/pages/2");') assert page.has_content?('Hello Alice') + # Sometimes Turbolinks isn't available right away. We need to wait for a brief moment + # for phantomJS to finish loading javascript. + sleep(0.1) + page.execute_script('Turbolinks.visit("/pages/1");') assert page.has_content?('Hello Bob') From 54eb7d63150546b2811e7f056a54fd905b3fdc47 Mon Sep 17 00:00:00 2001 From: Alec Hipshear Date: Sun, 13 Jul 2014 16:23:22 -0400 Subject: [PATCH 7/7] Heisenbug: http://en.wikipedia.org/wiki/Heisenbug Really just trying to work around an incredibly annoying bug that only seems to crop up once in a while. --- test/view_helper_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/view_helper_test.rb b/test/view_helper_test.rb index 1a2a09aee..20bbeb243 100644 --- a/test/view_helper_test.rb +++ b/test/view_helper_test.rb @@ -68,7 +68,7 @@ class ViewHelperTest < ActionDispatch::IntegrationTest # Sometimes Turbolinks isn't available right away. We need to wait for a brief moment # for phantomJS to finish loading javascript. - sleep(0.1) + sleep(1) page.execute_script('Turbolinks.visit("/pages/1");') assert page.has_content?('Hello Bob')