Drupal.org test bot doesn't run our PHPUnit tests

I could swear that we already had an issue with a similar title somewhere on drupal.org. Can't seem to find it on google though :(

Alright, a patch that fixes all the test fails and moves the unit tests to the correct location.
In case you would like me to split the commits into separate issues, just tell me. Moving the unit tests to the correct location is a separate commit from the test fixes.

Another try, this time it should pass.

Removed another 'menu_link' dependency (which causes the getPath() on non-object error) which I forgot.
Moved 2 unit tests out of the testing folder. Testbot doesn't like them, locally (with the run-tests script) it at least runs but testbot doesn't even do that. Excluding these two tests (from the menu subfolder) should make the testbot run.

The unit tests are executed now and everything is green, nice. However, we're not quite done yet ;)

1. +++ b/src/DataType/DataTypePluginManager.php
   @@ -38,4 +39,79 @@ class DataTypePluginManager extends DefaultPluginManager {
   +  /**
   +   * Returns the custom data types.
   +   *
   +   * @return \Drupal\search_api\DataType\DataTypeInterface[]
   +   *   An array of data type plugins, keyed by type identifier.
   +   */
   +  public function getCustomDataTypes() {
   

   We should have an interface for that service and move the documentation to it, would also make unit testing a bit easer. but that can be done in a separate issue I think.

   Do we actually need *Custom* in the method names?

2. +++ b/src/DataType/DataTypePluginManager.php
   @@ -38,4 +39,79 @@ class DataTypePluginManager extends DefaultPluginManager {
   +      $data_type_plugin_manager = \Drupal::service('plugin.manager.search_api.data_type');
   +
   +      foreach ($data_type_plugin_manager->getDefinitions() as $name => $data_type_definition) {
   +        if (class_exists($data_type_definition['class']) && empty($custom_data_types[$name])) {
   +          $data_type = $data_type_plugin_manager->createInstance($name);
   

   $data_type_plugin_manager is now $this, you don't need to call the service anymore.

3. +++ b/src/DataType/DataTypePluginManager.php
   @@ -38,4 +39,79 @@ class DataTypePluginManager extends DefaultPluginManager {
   +    $types = NULL;
   

   you need to define this as a property ($this->customDataTypes) otherwise the caching isn't doing anything.

4. +++ b/src/Tests/Processor/TestItemsTrait.php
   @@ -112,4 +134,30 @@ trait TestItemsTrait {
   +  /**
   +   * Returns the dataTypePlugin.
   +   */
   +  public function getDatatypePlugin() {
   

   Don't return it, assign it to $this->dataTypePlugin. Same for results static cache.

   And then rename the method to setUpDataTypePlugin().

5. +++ b/src/Tests/Processor/TestItemsTrait.php
   @@ -112,4 +134,30 @@ trait TestItemsTrait {
   +    $data_type_manager->expects($this->any())
   +      ->method('getCustomDataTypes')
   +      ->will($this->returnValue(array()));
   +    $results_static_cache->expects($this->any())
   +      ->method('getResults')
   +      ->will($this->returnValue(array()));
   

   Maybe we will need to extend this at some point and make it more flexible, but it looks like none of the unit tests need anything to be returned right now, they just happen to call it.

6. +++ b/src/Utility.php
   @@ -73,11 +73,11 @@ class Utility {
       */
      public static function getDataTypeOptions() {
        $types = array();
   -    foreach (self::getDataTypeInfo() as $id => $info) {
   +    foreach (\Drupal::service('plugin.manager.search_api.data_type')->getDataTypeInfo() as $id => $info) {
          $types[$id] = $info['label'];
        }
   

   This method should then move as well.

7. +++ b/src/Utility.php
   @@ -284,7 +209,7 @@ class Utility {
          static::$dataTypeFallbackMapping[$index_id] = array();
   -      foreach (self::getCustomDataTypes() as $type_id => $data_type) {
   +      foreach (\Drupal::service('plugin.manager.search_api.data_type')->getCustomDataTypes() as $type_id => $data_type) {
   

   In the long term, I think this class should not have any method that call out to Drupal::service(), but I think we don't need to move everything here, the patch is big enough.

8. +++ b/tests/src/Unit/Plugin/Processor/TransliterationTest.php
   @@ -37,6 +37,7 @@ class TransliterationTest extends UnitTestCase {
    
   +    $this->dataTypePlugin = self::getDatatypePlugin();
   

   This isn't a static method, call it with $this->, same for all other calls.

   Also don't forget to remove the assignment when you don't return it anymore.

New patch, points 2, 3, 4, 6 and 8 should be improved.

Thing still open:
1. Interface for DatatypePlugin
2. Extend mock methods where needed
3. Remove Drupal::serivce() calls from Utility class
4. Move ResultsCache Mock into a separate set up function. (currently in setUpDataTypePlugin())

And an interdiff file which shows the last changes.

1. +++ b/src/DataType/DataTypePluginManager.php
   @@ -23,6 +23,18 @@ use Drupal\search_api\Utility;
    class DataTypePluginManager extends DefaultPluginManager {
    
      /**
   +   * The custom data types property.
   +   *
   +   * @var array $customDataTypes
   +   */
   +  protected $customDataTypes;
   +  /**
   +   * The data type options property.
   +   *
   +   * @var array $dataTypeOptions
   +   */
   +  protected $dataTypeOptions;
   +  /**
   

   should have an empty line between the two and only one between class and the first docblock.

2. +++ b/src/DataType/DataTypePluginManager.php
   @@ -77,7 +87,7 @@ class DataTypePluginManager extends DefaultPluginManager {
      public function getDataTypeInfo($type = NULL) {
   -    $types = NULL;
   +    $types = $this->customDataTypes;
   

   Not quite ;)

   Just remove $types, use $this->customDataTypes directly.

3. +++ b/src/Tests/Processor/TestItemsTrait.php
   @@ -135,9 +135,9 @@ trait TestItemsTrait {
   -  public function getDatatypePlugin() {
   +  public function setUpDataTypePlugin() {
   

   The function can be protected.

4. +++ b/src/Tests/Processor/TestItemsTrait.php
   @@ -157,7 +157,8 @@ trait TestItemsTrait {
   +    $this->dataTypePlugin = $data_type_manager;
   +    $this->resultsStaticCache = $results_static_cache;
   

   No need for temporary local variables, just assign to $this->.. directly.

New version, this one seems to take some time while testing, so probably some more issues after this test run.

Test summary
------------

Drupal\search_api_db\Tests\BackendTest                       158 passes                           
Drupal\Tests\search_api\Unit\Plugin\Processor\TokenizerTest   22 passes                                 
Drupal\Tests\search_api\Unit\Plugin\Processor\Transliteratio   3 passes                               
Drupal\Tests\search_api\Unit\Plugin\Processor\FieldsProcesso  13 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\AddURLTest       2 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\IgnoreCaseTest   4 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\StopwordsTest    8 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\HighlightTest   13 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\IgnoreCharacte  39 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\LanguageTest     2 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\AggregatedFiel   9 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\HtmlFilterTest  25 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\NodeStatusTest   3 passes                                      
Drupal\Tests\search_api\Unit\Plugin\Processor\RoleFilterTest   2 passes                                      
Drupal\search_api\Tests\Processor\ContentAccessTest           72 passes                                      
Drupal\search_api\Tests\Processor\RenderedItemTest            42 passes                                      
Drupal\search_api\Tests\Processor\ProcessorIntegrationTest    31 passes  21 fails  20 exceptions             
Drupal\search_api\Tests\ServerTaskUnitTest                    59 passes                                      
Drupal\search_api\Tests\LocalActionsWebTest                    6 passes   7 fails   3 exceptions             
Drupal\search_api\Tests\ServerStorageUnitTest                  6 passes                                      
Drupal\search_api\Tests\LanguageIntegrationUnitTest           35 passes                                      
Drupal\search_api\Tests\IndexStorageUnitTest                   5 passes                                      
Drupal\search_api\Tests\OverviewPageTest                      58 passes  47 fails  25 exceptions             
Drupal\search_api\Tests\CustomDataTypesUnitTest               17 passes   1 fails                            
Drupal\search_api\Tests\IntegrationTest                       18 passes  28 fails  10 exceptions             
Drupal\search_api\Tests\ViewsTest                             16 passes  10 fails   1 exceptions        

Test run duration: 46 min 18 sec

wow... :D
--
Edit:

IntegrationTest: mostly fields that can't be set, tedious but easy fix.

CustomDataTypesUnitTest: FAILED: The processed value matches the altered original value

+++ b/src/DataType/DataTypePluginManager.php
@@ -120,8 +121,7 @@ class DataTypePluginManager extends DefaultPluginManager {
   public function getCustomDataType($type) {
-    $custom_data_types = $this->getCustomDataTypes();
-    return isset($custom_data_types[$type]) ? $custom_data_types[$type] : NULL;
+    return isset($this->customDataTypes[$type]) ? $this->customDataTypes[$type] : NULL;
   }

This change is not correct, you still need to call the method.

Modified one method to call getCustomDataTypes().

After moving the getDataTypeOptions method, I forgot to update the usage. That caused the test fails for IntegrationTest. New patch, I hope it also fixes the other test :D.

Fixed the last test fail.

Some more improvements. Tests should still pass.

getCustomDataType() has been removed, getDataTypeInfo() has been fixed and extractField() has been changed to accommodate the usage of createInstance instead of getCustomDataType(). The tests should pass (I only ran IntegrationTest, the pickiest one :D)

#21 wasn't really fixing anything, it just made the tests pass. I still don't get what that tests are doing, apparently. Here is the latest patch. with #21 reverted. All tests should pass now.

+++ b/src/Tests/CustomDataTypesUnitTest.php
@@ -121,6 +121,7 @@ class CustomDataTypesUnitTest extends EntityUnitTestBase {
 
+    debug($name_field->getValues());

Left-over debug. Can be fixed on commit as well.

The data type API needs more work, but I think this is a step in the right direction. I'm not sure why there are hardcoded data types and custom plugin data types, I think those should be converted to plugins too, would allow to simplify the API a lot.

On custom vs. default data types: Ok, but the weird thing is that a number of those methods on the data type plugin manager are now overlapping with the getDefinition()/getDefinitions() methods but by going through multiple nested calls.. getDataTypeDefinitions() calls getCustomDataTypes(), which instantiates all the plugin objects only to get the definitions from them again.

And the naming is quite strange, it's a DataTypePluginManager, but actually defines custom data types only, so getCustomTypes() returns it's own stuff and getDataType* returns more..

We tried to limit how many changes we make here so we stopped making more changes there, that's also why we didn't inject stuff nicely.. it already called static methods that then again called into services before, it was just better hidden :)

Maybe open an issue to discuss the naming of those methods?

We didn't move the Menu unit test because they are broken :) One is completely empty and doesn't do anything, the other fails and is kind of disabled with @requires extension disabled, but that doesn't actually work.

Also, looks like your git isn't configured for renames, so the unit test move makes the patch huge :)