Improve config translation test coverage by adding a test module

I've started working on this. I am going to build a config entity to test this.

Unfortunately no... I had a look today and I must have deleted it at some point. In summary, I was going to write a module to add a config entity that could be tested by config translation, however I noticed that the config module already creates a very similar config entity.

So we shall write some test cases for that config entity until we need to extend it for full test coverage and then reassess.

Yay! There are tests!

I had a look at @vijaycs85's tests in config_translation module and then wrote a test based on ConfigTranslationUITest.

I chose to make the test in Czech as a kudos to DrupalCon Prague (please correct if bad Czech used...).

There are currently some changes that I made in the setUp() of the test because the dotted.default breaks the config schema (more info here #2100203: Make config entities use dots in machine names consistently).

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationConfigEntityUITest.php
@@ -0,0 +1,146 @@
+  public static $modules = array('config_translation', 'config_test');

I think I also need to enable the config_translation_test module because currently the config_test_entity_info_alter creates a new config entity that uses the same base class as another. I will try to find the issue that hopes to fix this.

Added config_translation_test to modules for the test.

#2100257: ConfigTranslationDeleteForm needs to implement getCancelRoute method needs to be covered too. Check for cancel link on confirmation page.

Looking at the patch itself I'm not sure this gives us a lot more confidence. We already have tests that are translating contact forms in ConfigTranslationUITest.php's testContactConfigEntityTranslation() as well as basic Views translation tests in testViewsTranslationUI(). Not sure testing yet another entity type would give us more confidence. If there are additional asserts here (and looks like there are), IMHO those would be great to add to existing tests. We may also want to reorganize our tests a bit if the location of things being tested is not clear.

I talked to @alexpott today about this module and what should still be tested and he pointed out we should implement our own hooks in a test module at the least and ensure it works. I think the only hook we don't implement in the module is the alter hook for config translation info. We could easily test that altering that and adding another CMI key to a page works.

[11:46am] alexpott: GaborHojtsy: it's important that any new hooks that the module creates are at least used somewhere in core - even if that is only by a test module
[11:46am] GaborHojtsy: vijaycs85: hi
[11:46am] GaborHojtsy: alexpott: ok 
[11:46am] alexpott: GaborHojtsy: so if we can get that done I'm very +1 for getting it in
[11:47am] alexpott: GaborHojtsy: is there an issue for adding the test module?
[11:48am] GaborHojtsy: alexpott: https://drupal.org/node/2095383
[11:48am] Druplicon: https://drupal.org/node/2095383 => Improve test coverage by adding a test module #2095383: Improve config translation test coverage by adding a test module => Configuration translation for Drupal 8, Code, normal, needs work, 8 comments, 5 IRC mentions

Here is extending existing tests with conditions that were not yet there from @yanniboi's tests. Not yet covering delete and not testing the alter hook yet either.

Fixed those PHP errors. Evidently I did not run these tests locally :) Let's see what testbot thinks.

Small variable naming fix

I'm on this. Have fixes and extending the tests.

Added config file checking as well as test coverage for deleting translations on entities as I pointed out above.

Expanded with alter hook implementation in test module. That does not pass yet, the alter hook does not seem to be invoked proper... Also documented the type alter hook which does have a use case in the module itself but still needs some test coverage to ensure the textarea is properly used :) That should be a small assert.

Now with fixes for one of the fails and type testing for the fields, which tests our hook_config_type_info_alter() implementation. That did not yet have test coverage yet either as pointed out above. IMHO once this is made to pass, we have as complete test coverage as we can based on what was brought up so far.

Rolled the updated patch wrong :/ Here with all the changes. Same as #22 should have been, interdiff applies to this one.

A bit of a scope creep but menu item types are irrelevant for quite some time, and since this crept into the hook docs, etc. we should remove this sooner or later.

Helps to remove that from the interface :)

Fixed block fail that was due to #2043527: Theme name is included in block machine name but should be stored as a key instead and the other fatal that was due to still remaining call info menu item type that is not needed anymore.

So #2043527: Theme name is included in block machine name but should be stored as a key instead did not only fail our tests, it actually broke block translatability with our automations due to mismatches in IDs.

Looks like still need to specify menu item type for contextual links...

Also temporarily removing the alter hook tests so we can get to a passing patch and commit it to avoid conflicts elsewhere.

Uhm, that does not make the contextual links work, but I *really* want to get this in to continue :) Committing this.

Committed that, so what is left is making the contextual links work again and figuring out why the alter does not work :/ No interdiff, because this is basically the inverse of the interdiff in #32 and #33 above.

Here we go. I think the problem was that the cache was not sufficiently cleared after enabling the module. Instead of making that work, however, I think we should just enable the module in the entire test and then change the alter hook to only conditionally alter the information based on a state value. That is more standard I think.

Also some of the assertions were wrong I think. At least I was seeing different texts when testing this locally.

We'll see what the bot says.

So the altering works, it's just the contextual links that don't work. I'm not sure how they ever worked, though. I looked through the views contextual links code a bit, and it seems that one needs to implement hook_views_plugins_display_alter(). That's what views_ui.module does, at least. See views_ui_views_plugins_display_alter(). I'll investigate a bit more, tomorrow.

OK, this should work.

Forget #39 I totally didn't understand how contextual links work :-) I do now, sort of. So the menu links we add need to be of type MENU_SIBLING_LOCAL_TASK, not MENU_CALLBACK.

For anyone interested:
If they are MENU_CALLBACK they are not MENU_IS_LOCAL_TASK, and _menu_router_build() therefore sets '_tab' to FALSE. It then later checks if '_tab' is FALSE and in that case it unsets 'tab_parent'. menu_contextual_links(), however, queries all items that have the specified tab_parent, so the links don't show up.

We don't need the config_test module enabled in this test, so no need to add that. Also trying to re-enable the block tests, although I have a sense that the fix in #2043527: Theme name is included in block machine name but should be stored as a key instead was not enough for us (not fixed schema).

Ok, let's roll without that for now, since that is indeed not fixed yet :/ #2043527-51: Theme name is included in block machine name but should be stored as a key instead

All right, committed this! Also opened #2113989: Re-enable blocks testing once core bug is fixed for restoring the block test, so I think we did what this issue was supposed to do :) Now for further cleanups.

Committed as I said :)

Well, config_translation_test has a dependency on config_test, that's why I added it. But apparently simpletest automatically enables dependencies.

Yes, it does :)

Tagging.

Updated issue summary.