Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Test suite regressions in 3-2-stable #8068

Closed
mperham opened this Issue · 16 comments

3 participants

@mperham

I'm running my application's test suite against 3-2-stable and seeing a few test failures for a suite that ran green on 3.2.6:

Failures:

  1) Admin::BrandsController CREATE brands should generate the new action
     Failure/Error: response.should render_template 'form'
       expecting <"form"> but rendering with <"admin/brands/_brand_story_images, admin/brands/_form, admin/brands/edit, layouts/admin/unicorn">
     # ./spec/controllers/admin/brands_controller_spec.rb:24:in `block (3 levels) in <top (required)>'

  2) Admin::SalesEmailsController GET show renders the show template
     Failure/Error: response.should render_template "show"
       expecting <"show"> but rendering with <"admin/sales_emails/show_et">
     # ./spec/controllers/admin/sales_emails_controller_spec.rb:50:in `block (3 levels) in <top (required)>'

  3) BasketsController show with full cart handles HTML
     Failure/Error: response.should render_with_layout 'static'
     NoMethodError:
       undefined method `response' for #<ActionController::TestResponse:0x007f9bec362ec0>
     # ./spec/controllers/baskets_controller_spec.rb:29:in `block (4 levels) in <top (required)>'

  4) BasketsController show with full cart handles JS
     Failure/Error: response.should_not render_with_layout
     NoMethodError:
       undefined method `response' for #<ActionController::TestResponse:0x007f9be5b42f70>
     # ./spec/controllers/baskets_controller_spec.rb:36:in `block (4 levels) in <top (required)>'

  5) PresentationsController#show_all_styles displaying the correct layout displays the classic layout if no brand story media exists
     Failure/Error: response.should render_template("banner_classic")
       expecting <"banner_classic"> but rendering with <"presentations/_banner_classic, presentations/show_all_styles, layouts/_optimizely, layouts/_ie_styles, layouts/_kiss_metrics, layouts/_menu, layouts/_header, static/_footer, layouts/_crazy_egg, layouts/_tracking_js, layouts/presentations">
     # ./spec/controllers/presentations_controller_spec.rb:77:in `block (4 levels) in <top (required)>'

  6) PresentationsController#show_all_styles displaying the correct layout displays the images layout if there are one or more brand story images
     Failure/Error: response.should render_template("banner_brand_story_images")
       expecting <"banner_brand_story_images"> but rendering with <"presentations/_banner_overlay, presentations/_banner_brand_story_images, presentations/show_all_styles, layouts/_optimizely, layouts/_ie_styles, layouts/_kiss_metrics, layouts/_menu, layouts/_header, static/_footer, layouts/_crazy_egg, layouts/_tracking_js, layouts/presentations">

I'm not sure what caused the render_template failures but it's easy to argue that's our bug since it looks like we weren't actually looking for the right template name.

Here's the code for #3 and 4:

    context 'with full cart' do
      before do
        Factory(:cart_item, :cart => @cart, :quantity => 3)
        Factory(:cart_item, :cart => @cart, :quantity => 2)
        Factory(:cart_item, :cart => @cart)
      end

      it 'handles HTML' do
        get :show, :format => :html
        response.should render_template('show')
        response.should render_with_layout 'static'
        response.body.should =~ /\$#{60 + Cart::SHIPPING_COST}/
      end

      it 'handles JS' do
        get :show, :format => :js
        response.should render_template('show')
        response.should_not render_with_layout
      end
    end

Any idea of cause?

@rafaelfranca

#1, #2, #5 and #6 are caused by 7d17cd2

#3 and #4 I'm not sure yet. I'll investigate later.

@spastorino
Owner

@mperham can you test against current 3-2-stable? Any chance for you to bisect from the last commit until 3.2.6 ? Thanks

@mperham

@spastorino I can't easily because then my test fails with:

undefined method `to_i' for true:TrueClass
# /Users/mperham/src/rails/activerecord/lib/active_record/connection_adapters/column.rb:78:in `type_cast'
@rafaelfranca

@mperham have you tried to revert 7d17cd2 and see if everything pass?

@mperham

@rafaelfranca I'm sure that will fix the four. It does not fix the other two.

@mperham

Ok, I hacked the code and got git bisect working. It says the first bad commit is 2bad605

@spastorino
Owner

@rafaelfranca I wouldn't commit this kind of things to stable branches, so consequently I'd revert it. Thoughts? any other idea? :heart:

@rafaelfranca

Those commits are fixing bugs in stable branches, so if we revert this we will put the bugs back, also the regression will still exist in master branch.

I prefer to fix the cause instead of reverting. I'd only revert if the cause is not in Rails itself but in related projects like rspec.

@rafaelfranca

Between is this render_with_layout defined by rspec?

@rafaelfranca

@mperham oh, thank you. I will revert since I don't want to break any related projects in stable releases.

@rafaelfranca rafaelfranca referenced this issue from a commit
@rafaelfranca rafaelfranca Revert "Merge pull request #7797 from senny/7459_prefix_tempalte_asse…
…rtion_variables"

This reverts commit 2bad605.

Conflicts:
	actionpack/CHANGELOG.md

Reason: This added a regression related with shoulda-matchers, since it
is expecting the instance variable @layouts

See https://github.com/thoughtbot/shoulda-matchers/blob/9e1188eea68c47d9a56ce6280e45027da6187ab1/lib/shoulda/matchers/action_controller/render_with_layout_matcher.rb#L74

This will introduce back #7459 but this stable release will be backward compatible.
Related with #8068.
6b7cd20
@rafaelfranca rafaelfranca closed this issue from a commit
@rafaelfranca rafaelfranca Revert "Merge pull request #7659 from HugoLnx/template_error_no_match…
…es_rebased"

This reverts commit 7d17cd2.

Conflicts:
	actionpack/CHANGELOG.md

Reason: This added a regression since people were relying on this buggy behavior.
This will introduce back #3849 but we will be backward compatible in
stable release.

Fixes #8068.
d5b275d
@rafaelfranca

Should be all good now.

But I strongly recommend to fix your tests in #1, #2, #5 and #6 they are relying in buggy behavior of assert_select and will break again when you try to upgrade to 4.0.0

@mperham

Yes, I don't mind breaking APIs in major or minor releases. That's just part of the upgrade cycle. Just not in patch releases.

@rafaelfranca

:+1:, thank you for testing the release candidate :heart:

@mperham

Ok, theclymb.com is running green on 3-2-stable, thanks guys! :cake:

@spastorino
Owner

@mperham thank you for testing and helping :beers:

@gabebw gabebw referenced this issue in thoughtbot/shoulda-matchers
Closed

`@templates` will probably go away after Rails 3.2.9 #193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.