Remove/segment duplicate test coverage from ManageFieldsUiTest

LGTM

I am thinking we actually need to dissolve ManageFieldsUiTest entirely now, it's a monster.

I had another look at what's in there, and decided it would be worth also splitting out testCrudFields(), which was almost entirely added in 2009 to its own class called ManageFieldsLifecycleTest mostly because it looks weird and I had a suspicion it might be one of the slower tests and possibly entirely redundant at this point.

Then I opened up ManageFieldsTest and it is almost exactly the same, doing nearly the same things, just with entity_test module instead of Basic Page, Article, Taxonomy and a tags bundle. It's like over the years people have looked at ManageFieldsUiTest, closed it again, and created new test classes instead.

Roughly what I think we could do:

1. Commit this issue as-is or very close to it, it gives us more manageable chunks to look at.

2. Open a follow-up to merge the new (but in fact ancient) ManageFieldsLifecycleTest into ManageFieldsTest, adapting anything missing to fit into the more modern test structure there, then removing the class.

3. Same as 2 for ManageFieldsMultipleTypes tests.

3. Open another follow-up to rationalise the rest of ManageFieldsUiTest again with the intention of merging it or splitting it out into logical groups of classes with the rest of the field_ui test coverage.

Need to take it in smallish steps so we don't end up losing coverage, but from what I can see we're testing the same things two, three or four times in very slightly different ways when one would suffice.

Like the agile approach of incremental improvements.

Tagging for followups to be created for 6.2, 6.3

Moving to NW for cc failure and any additional work to make it management able for the follow ups.

Opened the follow-ups:

📌 Ensure ManageFieldsMultipleTypes test coverage is covered by ManageFieldsTest then delete it Needs work
📌 Ensure ManageFieldsLifecycleTest::testCrudFields() test coverage is covered by ManageFieldsTest then delete it Needs work

Fixed the missing method.

Marking but @catch what needs a follow up?

Not sure why the tag got added, if anything that comment should have removed it.

I'm not sure we should be removing \Drupal\Tests\field_ui\Functional\ManageFieldsFunctionalTest::testCRUDFields unless we have another test class that has extensive test coverage similar to this. I have been running this test consistently when I have been working on issues like 📌 Save FieldStorageConfig at the same time as FieldConfig Fixed . However, from DX perspective it seems good idea to move the CRUD test to it's own class.

Removing \Drupal\Tests\field_ui\Functional\ManageFieldsFunctionalTest::testDeleteTaxonomyField seems like we might be losing test coverage. I'm not sure which test is covering the non-node field deletion use case now? 🤔

I'm not sure which test is covering the non-node field deletion use case now?

FieldUIDeleteTest

:)

I'm not sure we should be removing \Drupal\Tests\field_ui\Functional\ManageFieldsFunctionalTest::testCRUDFields

Yes that's only for looking at in the follow-up, this MR only moves it to its own test class.

But isn't \Drupal\Tests\field_ui\Functional\FieldUIDeleteTest::testDeleteField testing only with nodes or am I missing something?

No it's me that's missing something, it's yet another test just using nodes.

Restored that coverage, we can try to replace it non-node coverage from ManageFieldsTest in the follow-ups.

lauriii → closed merge request !4907

Committed 753bac6 and pushed to 11.x. Thanks!

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