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

Add test coverage for rich_text_area helper #50255

Merged

Conversation

seanpdoyle
Copy link
Contributor

Motivation / Background

Follow-up to #50252

Similar to the reliance on a FormBuilder in the helper methods documentation examples, the template test coverage for #rich_text_area relied on invocations through a FormBuilder instance.

Detail

This commit adds explicit coverage for calling the #rich_text_area helper method directly with both an object_name and method_name positional arguments.

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.

actiontext/test/template/form_helper_test.rb Outdated Show resolved Hide resolved
output_buffer
end

test "#rich_text_area helper renders the value: argument into the hidden field" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to not combine this test with the "#rich_text_area helper" test?

Also, if we're keeping this test separate, I think the convention is to write the symbol form of option names:

Suggested change
test "#rich_text_area helper renders the value: argument into the hidden field" do
test "#rich_text_area helper renders the :value option into the hidden field" do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonathanhefner I kept them separate because I wanted to explicitly test the method signature. With rich_text_area_tag, the value is a positional argument. With rich_text_area, the value is derived from options[:value]. That different felt significant enough to cover separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are saying you want to keep this test separate from "#rich_text_area_tag helper", which I agree with. But I was asking why not combine this test and "#rich_text_area helper"?

Or am I misunderstanding what you wrote?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for being unclear. To complete the thought I state above, I think it's important to include a #rich_text_area test that omits the value and passes an HTML input option _so that there's coverage that the Hash isn't treated as a value positional argument.

Similarly, having the second test for explicit treatment of the value option feels similarly valuable to keep.

Does that clarify my original comment?

actiontext/test/template/form_helper_test.rb Outdated Show resolved Hide resolved
@seanpdoyle seanpdoyle force-pushed the action-text-rich-text-area-helper-test branch from e87066e to 917b4bb Compare December 3, 2023 18:47
@seanpdoyle seanpdoyle force-pushed the action-text-rich-text-area-helper-test branch from 917b4bb to d1fb0b8 Compare December 3, 2023 19:32
Follow-up to [rails#50252][]

Similar to the reliance on a `FormBuilder` in the helper methods
documentation examples, the template test coverage for `#rich_text_area`
relied on invocations through a `FormBuilder` instance.

This commit adds explicit coverage for calling the `#rich_text_area`
helper method directly with both an `object_name` and `method_name`
positional arguments.

[rails#50252]: rails#50252
@seanpdoyle seanpdoyle force-pushed the action-text-rich-text-area-helper-test branch from d1fb0b8 to 16c28d0 Compare December 3, 2023 19:53
@jonathanhefner jonathanhefner merged commit 41edc5a into rails:main Dec 3, 2023
3 of 4 checks passed
@jonathanhefner
Copy link
Member

Thank you, @seanpdoyle! 🧪

@seanpdoyle seanpdoyle deleted the action-text-rich-text-area-helper-test branch December 3, 2023 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants