Spin-off from #2864026: Convert web tests to browser tests for config module

The getAllOptions() method doesn't exist in AssertLegacyTrait, for ease of conversion it should be added.

From #2864026: Convert web tests to browser tests for config module by @jonathan1055:

With regard to copying/converting getAllOptions() to AssertLegacyTrait, it is only used in core tests in these two places (config and comment). I have searched the 28 modules for 8.x that I have on my local drive and only found it used once, in Token module.

It's not used a lot but it is used, so that is my case for adding it.

The logic in \Drupal\simpletest\AssertContentTrait::getAllOptions can probably be replaced by something like
$options = $element[0]->findAll('xpath', '//option');

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lendude created an issue. See original summary.

Lendude’s picture

Status: Active » Needs review
FileSize
2.89 KB

So something like this.

Lendude’s picture

Bleh, should have taken 5 seconds longer, moved the test to a more logical place.

jonathan1055’s picture

This looks fine to me. I have used it in #2866513-52: Convert web tests to browser tests for comment module #52 and it works.

I was happy to just use the direct one-liner and not add the method to assertLegacyTrait, but if you think this is the best way, that's OK too. Klausi was not keen on it, see #2863267-18: Convert web tests of views point 2, so it needs someone with more knowledge than me to set this to RTBC.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Lendude’s picture

Issue tags: +blocker

Thanks @jibran, tagging as blocker

  • xjm committed d2f5b62 on 8.5.x
    Issue #2907485 by Lendude, jonathan1055: Add getAllOptions() to...

xjm credited xjm.

xjm’s picture

Status: Reviewed & tested by the community » Fixed
Related issues: +#2908391: Add a rule for expected format of @deprecated and @trigger_error() text

Thanks, I can see the case for adding this for minimal test conversion noise and for non-core tests.

The deprecation message doesn't follow our usual format as documented in https://www.drupal.org/core/deprecation. I checked and saw that there are already lots of other places in core that also don't, so not knocking the issue back for that, but it does matter because we will eventually want to add a coding standard rule for this. I filed #2908391: Add a rule for expected format of @deprecated and @trigger_error() text.

Committed to 8.5.x. API additions normally go into the minor branch only, but we've already established a practice of backporting the legacy BC layer for tests in order to allow conversions across both branches, so I also cherry-picked it to 8.4.x. Thanks!

  • xjm committed 02de726 on 8.4.x
    Issue #2907485 by Lendude, jonathan1055: Add getAllOptions() to...
jonathan1055’s picture

Thanks Lendude, jibran, xjm.
I will now re-roll patch 52 from #2866513-52: Convert web tests to browser tests for comment module

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.