Problem/Motivation

When the underlying structure of Field UI changes, a lot of tests need to be updated. That's very boring and time consuming.

Proposed resolution

We should get smarter at testing Field UI.

Remaining tasks

Review the patch.

User interface changes

Nope.

API changes

A new FieldUiTestTrait trait is added and used by most tests that interact with Field UI.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because ... it is not a bug?
Issue priority Normal because ... it is neither critical nor major?
Unfrozen changes Unfrozen because it only changes tests.
Prioritized changes The main goal of this issue is to maintain the sanity of the person working on #1963340: Change field UI so that adding a field is a separate task
Disruption None at all.
CommentFileSizeAuthor
#1 2384357.patch26.28 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
26.28 KB

This should do it.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Way cool !
RTBC if green :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: 2384357.patch, failed testing.

Status: Needs work » Needs review

amateescu queued 1: 2384357.patch for re-testing.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! :)

Wim Leers’s picture

Nice!

webchick’s picture

Not moving down from RTBC, but using a trait seems a bit non-standard compared to how we typically do this. Typically, we would create a base class and have all related test classes derive from that. I see that we used to have a FieldUiTestBase class, but this patch removes it in favour of a trait. Why can we not just fix the base class to do the same thing?

amateescu’s picture

@webchick.. I'm still trying to decide if you're just trolling me, but, since I don't see any smileys there, I'll take it as an honest question and I'll do my best to provide a serious answer :)

Sure, "typically" that was the standard in Drupal 7 / PHP 5.2 era, but since we have adopted a modern PHP version which gave us the possibility to use traits, well.. what this patch is doing is the current standard (hint: can also be seen from the proposed resolution in the issue summary "We should get smarter at testing...")

Let's take ContactSitewideTest as an example. Even the class name implies that it's testing various scenarios ranging across multiple interaction points, and one of them happens to be adding a field through the UI.

Now, this could've been easily done by just using some API calls, but it choses to do it through the UI in order to validate some things which simple API call wouldn't catch (e.g. that the 'field_ui_base_route' annotation of the Drupal\contact\Entity\Message entity class is correct/working).

Since this is just one (very) small part of what ContactSitewideTest is testing, the rest of it not touching Field UI at all, if anyone can give me one good reason why this class should extend FieldUiTestBase, I'll print this patch on paper and eat it :D

yched’s picture

@webchick : a base class would be fine for tests whose primary purpose is to test Field UI.
The trait allows code re-use for tests that are about other systems, and thus might have their own primary base class, but happen to create fields in the UI.

Another option would be to skip the trait and add those methods to WebTestBase, but that would feel like bloat IMO (and arguable, since field_ui is an optional module)

amateescu’s picture

Of course I couldn't stay serious and that @yched had a more concise answer :/ #sigh

joelpittet’s picture

@webchick thanks for asking that question, traits are quite new for anybody using a >2 year old production server. And thanks for both your two perspectives on their benefit in this case. The translation and link generator traits seem very intuitive, this is a bit more subtle imo

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

So we are already heavily using traits in simpletest. All content assertions come from the AssertContentTrait so that both KernelTestBase and WebTestBase can share these assertions without polluting TestBase. Also we have SchemaCheckTestTrait which adds config schema assertion testing. So the pattern is already used. Providing custom assertions and helpers in traits for general functionality added by a module is a great way to give other modules an easy way to test things that they depend on.

Committed 0df31af and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed 0df31af on 8.0.x
    Issue #2384357 by amateescu: Simplify Field UI testing
    

Status: Fixed » Closed (fixed)

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