-
Notifications
You must be signed in to change notification settings - Fork 35
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 ability to specify non-existent field for combobox #28
base: main
Are you sure you want to change the base?
Add ability to specify non-existent field for combobox #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch. Thanks! Can you add a test that would fail without these changes and pass with the changes, please?
Happy to merge after that.
Should it be system test? |
Either a system or unit/integration test would be fine. I don't think we have tests for the main component. But we do have tests for the listbox option presenter. You can create tests based on that, if you wanted to test the component by itself. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the tests! Left some comments after a first pass.
|
||
class HotwireCombobox::ComponentTest < ApplicationViewTestCase | ||
setup do | ||
@view = ActionView::Base.new(ActionController::Base.view_paths, {}, ActionController::Base.new) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we're inheriting from ApplicationViewTestCase
the view object should be accessible by just calling #view
anywhere in the test cases. No need to initialize here.
We use this in the Listbox::Option
tests in the #render
method.
form: mock_form(form_object: OpenStruct.new("#{field_name}": existent_field_on_model)), | ||
) | ||
|
||
assert_equal component.hidden_field_attrs, { id: "#{field_name}_id-hw-hidden-field", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of asserting all attrs, let's just assert that the attr hash includes the correct name and value.
@@ -10,6 +10,21 @@ def assert_attrs(tag, tag_name: :input, **attrs) | |||
assert_match /<#{tag_name}.* #{attrs}/, tag | |||
end | |||
|
|||
def mock_form(form_object: OpenStruct.new) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the instinct to stub this out in the spirit of encapsulation.
In this case I'd rather not stub the form builder methods since they don't "belong" to us.
Let's test the real thing instead. SQLite supports JSON columns. Let's add one to one of the fixtures instead and use that in the tests instead of mocking the form.
value: nil } | ||
|
||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end | |
end | |
existent_field_on_model = "Peter" | ||
field_name = :name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test is clear enough that we don't need these explaining variables. Let's inline the values instead. Same goes for the other tests.
It's okay to introduce a bit of harmless repetition in order to keep less things in mind while reading these tests 👍
Also, please rebase or merge |
Co-authored-by: Jose Farias <[email protected]>
Co-authored-by: Jose Farias <[email protected]>
The reason for this PR that sometimes model can have json type fields with nested attributes and calling
form.object.public_send(field_name)
on it will fail. I used same check as rails do hereAt the moment calling this fails