Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #6834 (closed defect: fixed)

Opened 2 years ago

Last modified 11 months ago

[PATCH] html_document not properly reset in integration tests between requests - breaks assert_select

Reported by: peter_marklund Assigned to: jamis@37signals.com
Priority: high Milestone: 1.2 regressions
Component: ActionPack Version:
Severity: normal Keywords:
Cc: jarkko

Description

This bug is related to earlier tickets 5448 and 4631 (search in Trac for html_document). The problem is that the line @html_document in the process methon in integration.rb has no effect since that instance variable is not copied over from the session to the testcase.

I wanted to add a test to reproduce the problem. However the test/controller/integration_test.rb stubs out the process method so I couldn't use that test case, at least not as it is.

The steps I used to reproduce were (on Mac OSX):

1) sudo gem install rails --source http://gems.rubyonrails.org --include-dependencies 2) rails test 3) cd test; rake rails:freeze:edge 4) ./script/generate controller Home; ./script/generate integration_test home 5) create a file app/views/home/foo.rhtml and app/views/home/bar.rhtml that contain the words foo and bar respectively. 6) Edit the test/integration/home_test.rb to be: 

require "#{File.dirname(__FILE__)}/../test_helper"

class HomeTest < ActionController::IntegrationTest
  def test_foo_and_bar
    get "/home/foo"
    assert html_document.root.to_s =~ /foo/i

    get "/home/bar"
    assert html_document.root.to_s =~ /bar/i    
  end
end

7) ruby test/integration/home_test.rb and watch the test fail. 8) Make the following change to integration.rb: 

Index: actionpack/lib/action_controller/integration.rb
===================================================================
--- actionpack/lib/action_controller/integration.rb	(revision 5717)
+++ actionpack/lib/action_controller/integration.rb	(working copy)
@@ -534,7 +534,7 @@
     # test instance.
     def copy_session_variables! #:nodoc:
       return unless @integration_session
-      %w(controller response request).each do |var|
+      %w(controller response request html_document).each do |var|
         instance_variable_set("@#{var}", @integration_session.send(var))
       end
     end

An alternative fix to the recurring problem with html_document would be to make the html_document method always recreate the document when invoked.

Attachments

assert_select_integration_fix.diff (0.6 kB) - added by ctm on 12/18/06 23:23:20.
Fixes bug that interferes with assert_select in integration tests

Change History

12/18/06 23:23:20 changed by ctm

  • attachment assert_select_integration_fix.diff added.

Fixes bug that interferes with assert_select in integration tests

12/18/06 23:27:23 changed by ctm

  • summary changed from html_document not properly reset in integration tests between requests - breaks assert_select to [PATCH] html_document not properly reset in integration tests between requests - breaks assert_select.

Oops. I'm new to TRAC and didn't mean to attach a file without a comment. I'm seeing the same problem Peter Marklund has reported, but his fix didn't work for me. When I tried his fix, an integration test that didn't use html_document died with an unexpected nil.

(in reply to: ↑ description ) 12/19/06 21:26:20 changed by peter_marklund

The workaround that I use until this is fixed is: 

if RAILS_ENV == 'test'
  module ActionController #:nodoc:
    module TestProcess
      # Work around Rails bug 6834
      def html_document
        HTML::Document.new(@response.body)
      end
    end
  end
end

12/27/06 20:08:15 changed by david

  • owner changed from core to jamis@37signals.com.

12/27/06 20:08:33 changed by david

  • keywords set to 1.2regression.

12/30/06 20:52:16 changed by jarkko

  • cc set to jarkko.

01/01/07 03:03:13 changed by minam

  • status changed from new to closed.
  • resolution set to fixed.

(In [5828]) Make sure html_document is reset between integration test requests (closes #6834)

03/04/07 13:44:55 changed by bitsweat

  • keywords deleted.
  • milestone changed from 1.2 to 1.2 regressions.

10/09/07 14:00:36 changed by colthorp

  • status changed from closed to reopened.
  • resolution deleted.

I ran into this bug in edge. html_document had one of the previous pages I visited, though response.body was correct. Making the change originally posted fixed the problem: 

Index: actionpack/lib/action_controller/integration.rb
===================================================================
--- actionpack/lib/action_controller/integration.rb	(revision 5717)
+++ actionpack/lib/action_controller/integration.rb	(working copy)
@@ -534,7 +534,7 @@
     # test instance.
     def copy_session_variables! #:nodoc:
       return unless @integration_session
-      %w(controller response request).each do |var|
+      %w(controller response request html_document).each do |var|
         instance_variable_set("@#{var}", @integration_session.send(var))
       end
     end

10/09/07 14:18:18 changed by colthorp

Though this fixed the particular test that exhibited the problem, it caused other tests to fail in TestProcess#html_document (line 456)

10/09/07 15:10:00 changed by colthorp

Upon further investigation, the underlying problem was that the define_method loop that is modified in assert_select_integration_fix.diff does not define get_via_redirect, which was the request that caused assert_select to fail. Adding get_via_redirect to the list of method names fixed the bug without introducing new ones. 

Index: /Users/colthorp/svn/fantasy_real_estate/frontend/vendor/rails/actionpack/lib/action_controller/integration.rb
===================================================================
--- /Users/colthorp/svn/fantasy_real_estate/frontend/vendor/rails/actionpack/lib/action_controller/integration.rb	(revision 1508)
+++ /Users/colthorp/svn/fantasy_real_estate/frontend/vendor/rails/actionpack/lib/action_controller/integration.rb	(working copy)
@@ -493,7 +493,7 @@
       @integration_session = open_session
     end
 
-    %w(get post put head delete cookies assigns xml_http_request).each do |method|
+    %w(get post put head delete cookies assigns xml_http_request get_via_redirect post_via_redirect).each do |method|
       define_method(method) do |*args|
         reset! unless @integration_session
         # reset the html_document variable, but only for new get/post calls

10/13/07 03:32:23 changed by nzkoz

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [7850]) Define get_via_redirect as well. Closes #6834 [colthorp]

10/13/07 03:34:27 changed by nzkoz

(In [7851]) Missed post_via_redirect. References #6834