Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#1 | 2384357.patch | 26.28 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedThis should do it.
Comment #2
yched CreditAttribution: yched commentedWay cool !
RTBC if green :-)
Comment #5
amateescu CreditAttribution: amateescu commentedThanks! :)
Comment #6
Wim LeersNice!
Comment #7
webchickNot 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?
Comment #8
amateescu CreditAttribution: amateescu commented@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 extendFieldUiTestBase
, I'll print this patch on paper and eat it :DComment #9
yched CreditAttribution: yched commented@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)
Comment #10
amateescu CreditAttribution: amateescu commentedOf course I couldn't stay serious and that @yched had a more concise answer :/ #sigh
Comment #11
joelpittet@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
Comment #12
alexpottSo 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.