Convert AJAX part of \Drupal\field_ui\Tests\ManageDisplayTest to JavascriptTestBase

Work in progress patch. I converted 1 of the 2 tests and can't get it to work yet. I have a lot of @TODO's for behavior I don't understand (yet) in there but wanted to share the progress.

Would love some early feedback on it as to why my first test fails.

Adding a drupalGet before the loading makes it pass. Makes little sense to me at the moment why that is.

The rest is just a little cleanup to make the method calls easier to trace.

Thanks for looking into it. It's very weird. I expect it to work even without the form submit but somehow it doesn't. The UI actually displays everything correctly. I don't really know at the moment how to fix it. For now I'll just accept the drupalGet and make the test work and convert the other test.

Nice code cleanup btw.

I updated the test and implemented the second test. This test has the same problems though.. The tests to pass right now so it's a start. Still some weird requirements that I can't explain with my limited knowledge of fields in D8.

For now this is all I can do and I'll continue working on other tests. I've already spend several hours trying to figure this out, maybe someone with more fields knowledge has some more insights?

Setting to proper status.

Rerolled #7, there was a conflict on core/modules/field_ui/src/Tests/ManageDisplayTest.php.

Testbot shows 25 coding standards errors: https://www.drupal.org/pift-ci-job/695080

Most are old-style array() declarations, which is unusual since we already fixed all those. That suggests the patch in #7 is out of date enough that the code being moved around doesn't reflect those array syntax fixes.

So one wonders what other changes aren't carried forward. Git log for ManageDisplayTest tells us that it's at least #2878369: Tests for local tasks in Manage Display are not correct which might be enough to fail a test or two. And maybe other changes since Nov. 2016.

That means that the code being moved into the trait here should be re-done so that we're sure we got all the changes.

This should be converted to WTB.

Reroll patch from #11 and fixed coding standards errors.

Well, this take me back :)

Removed the new trait and just moved the one method to the test class. Removed all the todo's, cause if it's green it's green. Removed the weird third party assertion that was checking for something to be empty when we wouldn't actually expect it to be.

Lets see what the bot makes of this...

Perfect! this looks ready to me. Thank you.

+++ b/core/modules/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
@@ -429,2 +393,69 @@
+    $machine_name = $assert_session->waitForElementVisible('css', '[name="label"] + * .machine-name-value');

Haha, what makes you change your mind? :P

+++ b/core/modules/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
@@ -0,0 +1,461 @@
+    $display_storage = $this->entity_type_manager->getStorage('entity_view_display');
...
+    \Drupal::service('module_installer')->uninstall(['field_third_party_test']);
...
+    $display = $display_storage->loadUnchanged($display_id);

Pro Tip: Be careful about using the variables like this. After module uninstall we have a new container and storage is still old.

#25.1 Haha! I was hoping you would spot that :-) In this case the actual machine name is never used (like it was in the other test), we just need the link to show up so we can click on it.

#25.2 Yeah there are a number of assertions/tricks in this test that don't belong in a functional test. But 1-1 conversion, and moving them to a kernel test and getting the site in the right state would be a bit of bloat.

Should have RTBCed 3 days ago sorry. :)

Awesome to see this ready!

I had a look and it seems patch has some (tiny) CS issues:

1. +++ b/core/modules/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
   @@ -0,0 +1,461 @@
   +  public static $modules = ['node', 'field_ui', 'field_test', 'field_third_party_test', 'block'];
   

   Nit: can we put these on a separate line each?

2. +++ b/core/modules/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
   @@ -0,0 +1,461 @@
   +  protected $type;
   

   Missing docblock

3. +++ b/core/modules/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
   @@ -0,0 +1,461 @@
   +
   

   Dont need this extra line.

4. +++ b/core/modules/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
   @@ -0,0 +1,461 @@
   +    // Make sure we can save the third party settings when there are no settings available
   

   Line over 80 chars :)

@Manuel Garcia thanks for taking a look. Feedback addressed, leaving it RTBC since its only cosmetic changes.

RTBC +1

Thanks @Lendude for addressing those :)
RTBC++

Fixed these on commit:

FILE: ...es/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
 23 | WARNING | [x] A comma should follow the last multiline array
    |         |     item. Found: 'block'
 69 | ERROR   | [x] Visibility must be declared on method
    |         |     "testFormatterUI"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Committed 0855561 and pushed to 8.7.x. Thanks!