search

germsvel / phoenix_test

PhoenixTest provides a unified way of writing feature tests -- regardless of whether you're testing LiveView pages or static (non-LiveView) pages.
https://hex.pm/packages/phoenix_test
MIT License
178 stars 23 forks source link

Find select option via 'exact=true' (breaking change) #118

Closed ftes closed 2 months ago

ftes commented 2 months ago

Find select option by exact match. Allows disambiguating options that share a common substring.

E.g.

<select>
  <option value="orc">Orc</option>
  <option value="other_orc">Other Orc</option>
</select>

Currently, there is no way to distinguish Orc and Other Orc (Found more than one element).

ftes commented 2 months ago

Should exact: true be the default? Since inputs are found by exact match on their label, that would be consistent.

That would be a breaking change.

germsvel commented 2 months ago

Should exact: true be the default? Since inputs are found by exact match on their label, that would be consistent.

Yeah, that's excellent question. And thanks for adding this. The need for exact: true makes total sense.

My guess is that we should make exact: true the default for select. That's my guess based on the "principle of least surprise". It's kind of gut feeling, but I always ask myself, "what would a user expect to be the default here?"

In this case, I think we typically write the full label name -- so we're typically trying to do exact matches. That's different from assertions, where I think we're typically trying to do an inexact match (which is why the default there is false)

I think it's okay that it's a breaking change. I try to avoid them, but I think this is a good reason for having one.

ftes commented 2 months ago

My guess is that we should make exact: true the default for select. That's my guess based on the "principle of least surprise". It's kind of gut feeling, but I always ask myself, "what would a user expect to be the default here?" In this case, I think we typically write the full label name -- so we're typically trying to do exact matches. That's different from assertions, where I think we're typically trying to do an inexact match (which is why the default there is false)

After giving it some thought, I agree. exact=true is a good default for finding select options. I updated the PR and also removed the option again - I think it should be fine to keep things simple and non-configurable for now.

germsvel commented 2 months ago

I ... also removed the option again - I think it should be fine to keep things simple and non-configurable for now.

@ftes oh no! I hope my previous comment didn't make you remove the configurable exact flag. I thought that was a good portion (and I liked your other PR introducing that too).

Since we're creating the breaking change here, it'd be nice to provide the exact option too (when we release this change), so that the upgrade path is fairly straightforward. If people want to keep the current behavior, then they can specify exact: false. I wouldn't want to release this change, break things for people, and not have a way for them to overcome those issues.

We don't have to introduce that exact flag here, but we should release it as part of the same set of changes. So, I'm gonna hold off on merging this until we have those other changes lined up too. If you feel like reopening your other PR and reintroducing them here, that'd be great. Otherwise, I can add those when I'm done working on the form-helpers-that-accepts-selectors issue.

ftes commented 2 months ago

Then I misunderstood you 😆 No problem, I'll re-add in this or another PR.

ftes commented 2 months ago

exact option re-added.

germsvel commented 2 months ago

Opened #127 to keep track of this work. Thanks again for getting this in!