Identifying the field error messages belong to

[G-Rath]

trafficstars

Is it possible for bootstrap_form to include information on an .invalid-feedback element about what field it belongs to?

Currently, .invalid-feedback elements are seemingly placed directly after ("under") the element they're about, which isn't really strong enough to safely target & test for in rspec/capybara.

I think this would work well as a css class, but could also be done using a data-for-style attribute.

[lcreid]

This is a really interesting idea. Thanks! I'm a big fan of testing, and of good system tests, so this strikes a chord with me. Unfortunately, I haven't had much time to look into this, so apologies for my slow response.

As much as I like the idea, we also have an informal design goal to try to minimize the markup that the bootstrap_form gem adds itself (as opposed to what's required by Bootstrap). Do you have any ideas about how this could be done without adding markup for users who didn't care about error messages in Capybara tests?

[G-Rath]

This is a really interesting idea.

Glad you think so - was a little worried it might not be so well received!

Do you have any ideas...

Sadly, I've only actually just started getting into Ruby/Rails, so it's all too new for me to really suggest anything.

Personally, I'd see this being useful outside of testing as well, as it'd be useful for targeting with ajax for doing things like updating error messages dynamically.

As a side note (and possible new feature request): I originally tried converting the form to use ajax, but found this annoying to attempt b/c bootstrap_form doesn't provide the .invalid-feedback elements unless there's an actual error on page render, meaning I'd have to use (extra) jQuery code to figure out if an feedback div existed for a specific input, append a new element if it didn't, and update the message. It would be cool if bootstrap_form supported a generate_empty_invalid_elements flag for when we want to do forms async. (Let me know if you're be interested in this, and I'll create a separate issue)

You could aim for checking the environment? In node you usually can check process.env.node_env, and that'll get set to production or development. I know rails does have test vs production vs development based environment settings, so that might be something you could use.

However, the counter argument for that is that you're actually changing the way the view is built based on if you're testing it or not, which technically means you're not testing the correct content?

I think the above is pretty much the reason it has to be done in markup, as otherwise you're not testing the actual end-result that's used in production, which is sort of the point of testing 😄

[lcreid]

OK. No worries. Hopefully this week I'll make a chance to think about this and see what others have done.

I think you second idea is also worth exploring, but I'd prefer to separate the two iisues. If you don't mind, could you please open another issue and cut and paste the text into the new issue? I agree with your comments about testing. We should generate the same code regardless of the environment, unless there's absolutely no other way to do it.

[lcreid]

I almost had myself convinced that the right thing was, in the Capybara test, to do something like this:

assert_selector "#desired_input_field.is-invalid + div.invalid-feedback", text: "expected text"

But I found lots of cases where the div.invalid-feedback doesn't immediately follow the input. Custom file inputs, radio buttons and check boxes, for example, where the label is in between the two. Also, any control that has an appended button or other content.

That said, I do believe that for each test case, you would know exactly what you were looking for, so you could write a selector that found the right div.invalid-feedback, I believe.

Also, the following appears to work in one case. I wonder if it would be reliable in the general case?

assert_selector "#desired_input_field.is-invalid ~ div.invalid-feedback", text: "expected text"

(Note the "~" instead of "+".)

I'm leaning towards the idea that we should document how to write selectors to test error messages, but I'm still open to other ideas. Perhaps we could do the documentation as a first step, and then see if people find cases where it isn't good enough? If they do, then we can consider adding mark-up to bootstrap_form's output.

[G-Rath]

Personally I really don't like this, as imo it makes the test brittle, and very design specific - it means you're writing tests that say "the invalid feedback field should be here, after element x, before element y on the dom", when really it's meant to be testing that an error message is shown on the page for the user to see.

At the end of the day you can do a lot of freaky things with the DOM in terms of positioning, so it's very possible to be in a situation where the rendering of DOM elements doesn't match their position on the tree.

That's one of the reasons why you shouldn't test for specific DOM elements being in specific places when doing UI testing - you should instead "render" the actual DOM, and test it's visual output (when doing the highest level of UX testing).

While I understand why it'd be nice to reduce the amount of output by the builder, I feel like this is fighting the system a bit - one of the entire points of css classes is to allow this sort of functionality.

Remember this also isn't limited to tests: It's risky to use such a selector in live when trying to dynamically update or otherwise adjust said elements, as it'd be very easy to end up selecting the wrong thing by mistake.

[lcreid]

I don't disagree with your comments. The way the form is rendered is flexible. But most people's use of Capybara isn't testing that.

Most importantly, I think, in this case, we're testing code that's generated by bootstrap_form, so the elements will always be in the same place in the DOM (at least until Bootstrap 5 comes along, if our experience going from 3 to 4 is repeated with 5).

I can imagine use cases where you might want to find the error message for a specific element outside of testing, as you mention at the end.

Do you think it would be good enough if there was simply an option on the helper to add attributes to the div.invalid-feedback? We could do something like have an invalid_wrapper option on the bootstrap_form helpers that takes a hash. The hash would set the attributes on the div.invalid-feedback, so that:

f.text_field(@name, invalid_wrapper: { id: "special", class: "bg-danger text-light" })

would generate:

<div class="invalid-feedback bg-danger text-light" id="special">

That way we'd only generate markup if someone needed it, and it would be their responsibility to ensure that the ID didn't conflict with any other ID on the page. They'd also be free to use class as a selector if that was more convenient that ID, and they could even use data attributes.

[G-Rath]

I think that'll do nicely, except I think it'd be worth having a form-level option about adding a class (or other attribute, maybe name: invalid[ ... ]) that's based on the name of the fields, to leverage the ability to be consistent and automatic.

Otherwise, you're having to remember to add that every field in your form, and to use the name of the field twice, like so:

<%= f.text_field :first_name, wrapper: { class: "col-sm-6" }, invalid_wrapper: { name: :first_name } %>

I feel like it would be a mistake not to take advantage of this to reduce on code and make things a bit easier for devs; otherwise, it's another thing to have to explain and ensure every single time.

[donv]

Hi!

This issue is now over 4 years old. Is there still interest?

@lcreid I see you add some labels to old issues. Is there a plan for this? What is the purpose?

[lcreid]

I see no problem keeping good ideas around in the issues, even if we don't have time to implement them. But if you prefer to close them, go ahead.