Rename ActionView::TestCase::Behavior::{Content,RenderedViewContent}

[seanpdoyle]

Motivation / Background

Closes #49818

Detail

Renames ActionView::TestCase::Behavior::Content to
RenderedViewContent, with the goal of making it more of an internal
implementation detail that's unlikely to collide with an
application-side ::Content class.

The RenderedView-prefix mirrors the module's RenderedViewsCollection
class. Since the intention is to treat it as a private implementation
detail, RenderedViewContent is marked with :nodoc:.

Along with the rename, this commit also modifies the class inheritance,
replacing the SimpleDelegator superclass with String. String.new
accepts a String positional argument in the same way as
SimpleDelegator.new accepts a delegate object positional argument.
Sharing the String superclass also makes it a good candidate for being
passed to Capybara.string (and Capybara::Node::Simple.new) like
the documentation suggests.

Additional information

Is this worth backporting into a 7.1 release?

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[zzak]

Technically, I think this is public API so it would require a deprecation cycle -- and that rules out backports. 🤔

[p8]

It was added 2 months ago: #49194
I think we could skip the deprecation cycle.

[zzak]

Sure, but it also exists in 2 public releases of Rails (7.1.0 and 7.1.1), you can also make an argument that is a bug.

I'm just trying to point out the technical detail that might be otherwise overlooked.

[p8]

I think we've made internal classes :nodoc: before without deprecation cycles.
Not sure if those were backported though 🤷

[seanpdoyle]

I'm happy to mark the class :nodoc: in addition to the rename. I was under the impression that it was internal from the start. That's my mistake for missing that detail in code review.

[p8]

I don't think RenderedViewsCollection and Locals should be public API either:
https://edgeapi.rubyonrails.org/classes/ActionView/TestCase/Behavior/RenderedViewsCollection.html
https://edgeapi.rubyonrails.org/classes/ActionView/TestCase/Behavior/Locals.html

[zzak]

RenderedViewsCollection was introduced 11 years ago in #7886, so I think that one definitely needs a deprecation cycle.

Locals seems to exist before that (it's visible in the diff).

Let's bring this up in discord and see what kind of feedback we get. #:pray:

[seanpdoyle]

Thank you for following up on this @zzak and @p8. While I hope that a backport is possible, will that discussion impact the code changes on this PR? Similarly, what sorts of code changes might be necessary if we'd need to deprecate the old class?

[zzak]

While I hope that a backport is possible, will that discussion impact the code changes on this PR?

Definitely, since deprecations aren't backported.

Similarly, what sorts of code changes might be necessary if we'd need to deprecate the old class?

Probably some combination of deprecate_constant and assert_deprecated.

[seanpdoyle]

@zzak @p8 I've changed this PR to be slightly more aggressive than it was originally.

While the Content class itself exists in the ActionView::TestCase namespace, test suites won't ever interact with that class directly. When their test inherits from ActionView::TestCase, Content (by way of the class-level .content_class attribute) gets duplicated. That means that the class that tests interact with isn't the same instance of Class that the Content constant references.

Does that quirk warrant skipping the deprecation process?

[zzak]

@seanpdoyle Could you expand this changelog to make sense in English? I understand what it means, but someone with limited bash-fu might not be looking for this

[seanpdoyle]

@zzak I've opened #50752.

[mockdeep]

@seanpdoyle this change caused some breakage in our view tests. When we render more than once in a test, now the later renders don't change the rendered result. It's not a huge deal, as we only do this in a couple of places, but just FYI. For example in the following test case, the second assertion fails because the view content hasn't updated:

  it 'renders whether bulk approval is enabled' do
    render 'admin/steps/list', local_assigns

    expect(rendered).not_to have_text('Bulk Approval is enabled')

    step.update!(bulk_approval_enabled: true)
    render 'admin/steps/list', local_assigns

    expect(rendered).to have_text('Bulk Approval is enabled')
  end

[rafaelfranca]

How do you know it was this change? It should not cause this since it only renamed a class.

[seanpdoyle]

I believe that change in behavior originates from https://github.com/rails/rails/pull/49194/files#diff-ce84a807f3491121a5230d37bd40454bb1407fcca71179e1a2fa76d4c0ddfa2aR293.

Is rendering in this way intended to be supported? I have a branch locally to remove the memoization with this change:

diff --git a/actionview/lib/action_view/test_case.rb b/actionview/lib/action_view/test_case.rb
index c12feeac95..006c6bf759 100644
--- a/actionview/lib/action_view/test_case.rb
+++ b/actionview/lib/action_view/test_case.rb
@@ -291,7 +291,7 @@ def rendered_views
       #     assert_pattern { rendered.json => { title: "Hello, world" } }
       #   end
       def rendered
-        @_rendered ||= self.class.content_class.new(@rendered)
+        self.class.content_class.new(@rendered)
       end
 
       def _routes
diff --git a/actionview/test/template/test_case_test.rb b/actionview/test/template/test_case_test.rb
index 163ec700d2..0f1f27b004 100644
--- a/actionview/test/template/test_case_test.rb
+++ b/actionview/test/template/test_case_test.rb
@@ -394,6 +394,20 @@ class RenderedViewContentTest < ActionView::TestCase
       assert_match(/#{developer.name}/, rendered)
       assert_includes rendered, developer.name
     end
+
+    test "#rendered resets after each render" do
+      render "developers/developer", developer: DeveloperStruct.new("First")
+
+      assert_equal "First", rendered
+
+      render "developers/developer", developer: DeveloperStruct.new("Second")
+
+      assert_equal "Second", rendered
+    end
   end
 
   class HTMLParserTest < ActionView::TestCase

The test is failing:

Failure:
ActionView::RenderedViewContentTest#test_#rendered_resets_after_each_render [test/template/test_case_test.rb:405]:
Expected: "Second"
  Actual: "FirstSecond"

Calling render more than once utilizes the same output buffer, which is behavior that this change did not introduce.

While I appreciate the idea of preserving backwards compatibility, I wonder: would that particular example of test coverage be better served as two separate tests with two output buffers?

[seanpdoyle]

@mockdeep @rafaelfranca I've opened #51093 in response to #49856 (comment).

[mockdeep]

@rafaelfranca We bisected the commits and landed on this one. It's a little confusing as to why this change would cause it, so maybe we missed something.

@seanpdoyle it's not critical behavior for us, and we'll probably end up splitting up the couple of tests that failed. We're working on phasing out view specs anyway. However, if it's not supported behavior, it seems like a second render should maybe throw an error to make it clear that it's not supported.

[seanpdoyle]

it seems like a second render should maybe throw an error to make it clear that it's not supported.

Upon further reflection, I do believe multiple renders is and should be supported.