Fatal Error: ViewUI::isTranslatable endless loop

That's definitive a critical bug.

Ah, this was #1945348: Call decorated storage methods in ViewsUI explicitly.

Maybe just a unit test to call each of these methods on the decorated object?

Oops, sorry :) would all of these methods work in a unit test? A dutb test?

Started to write a test which invokes the method directly and checks the invocation using a mock object.

According to http://stackoverflow.com/questions/3079918/phpunit-stubbing-class-method... it sadly has issues with final methods (isNew() on ConfigEntityBase). To be honest final always feels wrong in the land of drupal.

We could get rid of all array elements, which are methods without any parameters, because then we know the actual default values.

Note: You can't mock final methods. In general I think you should be able to override isNew() if you really want,
so let's remove it.

Looks really nice. Just a few notes:

+++ b/core/modules/views/tests/Drupal/Tests/ViewsUI/ViewUIObjectTest.php
@@ -0,0 +1,94 @@
+namespace Drupal\Tests\ViewsUI;

Should this be Drupal\views\Tests\ViewsUI?

+++ b/core/modules/views/tests/Drupal/Tests/ViewsUI/ViewUIObjectTest.php
@@ -0,0 +1,94 @@
+      'name' => 'View UI Object (unit test)',

I'm not sure if " (unit test)" is needed here.

+++ b/core/modules/views/tests/Drupal/Tests/ViewsUI/ViewUIObjectTest.php
@@ -0,0 +1,94 @@
+      $method_mock = $storage->expects($this->once())
+        ->method($method);
+      foreach ($args as $arg) {
+        $method_mock->with($this->equalTo($arg));
+      }

I think the args should be ->with(arg1, arg2, ...)

Should this be Drupal\views\Tests\ViewsUI?

That's a unit test, so the namespace is like that.

I think the args should be ->with(arg1, arg2, ...)

I tried this first, though we have a dynamic amount of arguments ... there is a foreach loop above, so we can't use that here.

Tried this with simplytest.me: enabled simpletest, went to testing, checked phpunit, hit run and got "Warning: call_user_func() expects parameter 1 to be a valid callback, class 'Drupal\Tests\ViewsUI\ViewUIObjectTest' not found in simpletest_result_form() (line 247 of core/modules/simpletest/simpletest.pages.inc)."

there is a foreach loop above, so we can't use that here.

That works since we have less than two arguments. Only second call would throw an exception from here.

+++ b/core/modules/views/tests/Drupal/Tests/ViewsUI/ViewUIObjectTest.php
@@ -0,0 +1,94 @@
+    // EntityInterface::isNew() is missing because it calls id() is called
+    // internally so handle it different.

Is this still true now that we can mock it?

Is this still true now that we can mock it?

Well what should I say, you can't remove a mock from my perspective. So this always counts the id() call.

Sorry for being unclear on that. I mean there is no need to handle isNew differently and we can replace that comment with $method_args['isNew'] = array();.

About the namespace: checked out breakpoint's unit test and it is using a namespace like Drupal\[module]\Tests.

The simpletest UI is simply borked with phpunit tests, see the breakpoint tests, thought yeah I agree that this is a better place for the test.

+++ b/core/modules/views/tests/Drupal/views/Tests/ViewsUI/ViewUIObjectTest.phpundefined
@@ -0,0 +1,94 @@
+ * Contains \Drupal\views\Tests\ViewsUI\ViewUIObjectTest.php

extra "php" on end.

+++ b/core/modules/views/tests/Drupal/views/Tests/ViewsUI/ViewUIObjectTest.phpundefined
@@ -0,0 +1,94 @@
+class ViewUIObjectTest extends UnitTestCase {

I looked at #1446366: [meta] Multiple web test classes mislabeled as unit tests. simpletest ([Web|Unit|DrupalUnit]TestBase) the other day.. so I thought maybe this should be UnitTest.

Should the file and the class name be changed to ViewsUIUnitTest ?
grep -R "Object" * | grep Test | grep -v vendor | grep extends
only gives:
core/modules/system/lib/Drupal/system/Tests/Form/FormObjectTest.php:class FormObjectTest extends SystemConfigFormTestBase {

So I didn't see a pattern of other ObjectTest's

Should be ViewsUI (not ViewUI)?
Wait, tim says in irc View singular is started to be used.

And points out this is a PHPUnit test and not a Drupal Simple Test.

I started to look at #1901670: Start using PHPUnit for unit tests to see what it says about the location and naming of tests... but then figured that must have been what was already discussed above and it just didn't sink in when I read the comments at first.

So leaving the name and location alone.

---
looks like this has tests, so removing the needs tests tag.

#14: drupal-1962130-14.patch queued for re-testing.

RTBC if this still passes.

Let's start to inject more things.

+++ b/core/modules/views/tests/Drupal/views/Tests/ViewsUI/ViewUIObjectTest.phpundefined
@@ -73,7 +80,6 @@ public function testEntityDecoration() {
     $method_args['getBCEntity'] = array();
-    $method_args['getOriginalEntity'] = array();
     $method_args['isTranslatable'] = array();

The method got renamed to getNGEntity. But I don't care about test coverage for them, both will only exist as long as EntityBC exists. So, back to RTBC.

Oh thanks.

As this might be a reason to not commit it, here is a fix.

+++ b/core/modules/views/tests/Drupal/views/Tests/ViewsUI/ViewUIObjectTest.phpundefined
@@ -0,0 +1,99 @@
+    $method_args = array();
+    $method_args['getOriginalId'] = array();
+    $method_args['setOriginalID'] = array(12);
+    $method_args['enable'] = array();
+    $method_args['disable'] = array();
+    $method_args['setStatus'] = array(TRUE);
+    $method_args['id'] = array();

Not really excited about this test. #19 already pointed out one area where we missed explicitly documenting an expected method here, and there will only be others as Drupal grows, and nothing to send people back here to further extend them. I guess it's ok to keep it in, but what we really need tests for us what the OP tried to do, which is edit a view with language module enabled. The fact that none of our existing tests caught this bug shows that there are probably other aspects of multilingual views that have bugs that we don't know about.

+++ b/core/modules/views/tests/Drupal/views/Tests/ViewsUI/ViewUIObjectTest.phpundefined
@@ -0,0 +1,99 @@
+    // EntityInterface::isNew() is missing because it calls id() is called
+    // internally so handle it different.

Sorry, I have NO idea what this is saying. Happy to work out some more English on IRC. :)

+++ b/core/tests/bootstrap.phpundefined
@@ -17,6 +17,9 @@
+// @todo Remove once views_ui has it's own directory.
+$loader->add('Drupal\\views_ui', __DIR__ . "/../modules/views/views_ui/lib");
+
 require __DIR__ . "/../../core/lib/Drupal.php";

This isn't going to fly... this introduces a workaround for Views UI module only, and not something that contrib modules (e.g. User Relationships et al) can use that *also* define modules in sub-folders. We need to fix this in a general way.

Removing the unit test and changing it to a functional test would allow us to defer that to later. Not sure if that's the best approach though.

Ok, I just committed #1820414: CHANGE NOTICE: Move views_ui.module directly into /core/modules/ so this will need a re-roll to remove that hunk.

Let's get a follow-up filed though for adding some general logic to core/tests/bootstrap.php that can handle modules like User Relationships.

Not really excited about this test. #19 already pointed out one area where we missed explicitly documenting an expected method here, and there will only be others as Drupal grows, and nothing to send people back here to further extend them. I guess it's ok to keep it in, but what we really need tests for us what the OP tried to do, which is edit a view with language module enabled. The fact that none of our existing tests caught this bug shows that there are probably other aspects of multilingual views that have bugs that we don't know about.

Well I think we need both. I tried to write some magic to iterate over all methods, though it caused issues, as it is not possible to find proper parameters for all the methods.

This test does exactly test just the class, which is the key of unit tests. You don't bother with all the crap around it,
so you test exactly one piece of code, well.

I hope the new text is easier to understand.

Here's a web test.

--- a/core/modules/views/lib/Drupal/views/Tests/UI/DefaultViewsTest.php
+++ b/core/modules/views/lib/Drupal/views/Tests/UI/DefaultViewsTest.phpundefined

Are you sure this is the proper place to test this?

Yeah, maybe Drupal\views\Tests\UI\SettingsTest or something?

What do you think about a test in translation_entity module?

let's apply some magic to fetch at least the methods without parameters.

Okay we now have a more sane unit test, and a web test!

Awesome, that looks great! Thanks a lot. Also, that Reflection stuff is COOL!

#27 B shows the test failing without the patch, so I think we're good to go here. :)

Committed and pushed to 8.x. Yeehaw!