Add more test coverage for node/file verbose fields.

Nice.

1. +++ b/src/Tests/MonitoringUITest.php
   @@ -95,31 +95,41 @@ class MonitoringUITest extends MonitoringTestBase {
        // Test details page by clicking the link in confirmation message.
   -    $this->assertLink(t('UI created Sensor'));
   -    $this->clickLink(t('UI created Sensor'));
   +    $this->assertLink(t('Node Entity Aggregator sensor'));
   +    $this->clickLink(t('Node Entity Aggregator sensor'));
        $this->assertText('Result');
   

   We should assert some of the values that we are expecting there.

2. +++ b/src/Tests/MonitoringUITest.php
   @@ -128,11 +138,51 @@ class MonitoringUITest extends MonitoringTestBase {
   +    // Test details page by clicking the link in confirmation message.
   +    $this->assertLink(t('File Entity Aggregator sensor'));
   +    $this->clickLink(t('File Entity Aggregator sensor'));
   +    $this->assertText('Result');
   

   Same here.

Take a look at the verbose output for the pages you test, there is no verbose output for the node sensor, but the test passes anyway. All the fields you assert aren't actually displayed, they are only listed as a setting in the Settings detail inset. I attached a screenshot of the verbose table which is empty.

+++ b/src/Tests/MonitoringUITest.php
@@ -128,11 +156,64 @@ class MonitoringUITest extends MonitoringTestBase {
+      $this->drupalPostForm(NULL, array('settings[entity_type]' => 'node'), t('Add another field'));
...
+    $this->assertLink($node->getTitle());
+    $this->assertLink($node->getOwner()->getUsername());
+    $this->assertText($node->uuid());
+    $this->assertText(\Drupal::service('date.formatter')->format($node->getCreatedTime(), 'short'));
+    $this->assertText($node->getChangedTime());

You are using the node for the file sensor? I'd expect you create a file, create the sensor and check the sensor output to match what the file has. At the moment the file sensor is just a copy of the node sensor with rather useless output (the output of a node, after all :P). Take a look at this test to see how to create a file in a test and do stuff with it.

+++ b/src/Tests/MonitoringUITest.php
@@ -91,36 +91,64 @@ class MonitoringUITest extends MonitoringTestBase {
+    $edit = array();
+    $edit['description'] = 'Sensor created to test UI';
+    $edit['value_label'] = 'Test Value';
+    $edit['caching_time'] = 100;
+    $edit['settings[aggregation][time_interval_value]'] = 86400;
+    $edit['settings[entity_type]'] = 'node';
+    $edit['conditions[0][field]'] = 'type';
+    $edit['conditions[0][value]'] = 'article';
+    $edit['conditions[1][field]'] = 'sticky';
+    $edit['conditions[1][value]'] = '0';

i would recommend merging it into one single declaration, ex: array( 'key'=> 'value', 'key2'=> 'value'). Just the way it was before. will have a look about failing tests.

And about the test, are failing because of some recent changes in the Search API module, has nothing todo with this patch ;)

The fails are because of a dependency on SearchAPI which was broken by a core update. The issue to fix SearchAPI: #2545464: Fix database backend after recent core change.

1. +++ b/src/Tests/MonitoringUITest.php
   @@ -91,36 +91,64 @@ class MonitoringUITest extends MonitoringTestBase {
   -    $this->assertLink(t('UI created Sensor'));
   -    $this->clickLink(t('UI created Sensor'));
   +    $this->assertLink(t('Node Entity Aggregator sensor'));
   +    $this->clickLink(t('Node Entity Aggregator sensor'));
   

   I see this is from before, but we don't need to assert link before clicking to it.

2. +++ b/src/Tests/MonitoringUITest.php
   @@ -128,11 +156,83 @@ class MonitoringUITest extends MonitoringTestBase {
   +    $contents = "file_put_contents().";
   

   Looks a bit strange, but okay. :)

3. +++ b/src/Tests/MonitoringUITest.php
   @@ -91,36 +91,64 @@ class MonitoringUITest extends MonitoringTestBase {
   +    $i = 2;
   +    foreach ($node_fields as $field) {
   +      $this->drupalPostForm(NULL, array(), t('Add another field'));
   +      $edit['settings[verbose_fields][' . $i++ . ']'] = $field->getName();
   +    }
   +    $this->drupalPostForm(NULL, $edit, t('Save'));
   
   @@ -128,11 +156,83 @@ class MonitoringUITest extends MonitoringTestBase {
   +    $i = 2;
   +    foreach ($file_fields as $field) {
   +      $this->drupalPostForm(NULL, array(), t('Add another field'));
   +      $edit['settings[verbose_fields][' . $i++ . ']'] = $field->getName();
   +    }
   +    $this->drupalPostForm(NULL, $edit, t('Save'));
   

   What about adding a helper method (e.g. addFields())?

4. +++ b/src/Tests/MonitoringUITest.php
   @@ -128,11 +156,83 @@ class MonitoringUITest extends MonitoringTestBase {
   +    $this->assertTrue($file->id() > 0);
   

   Can we use $this->assertTrue($file->id())?

5. +++ b/src/Tests/MonitoringUITest.php
   @@ -128,11 +156,83 @@ class MonitoringUITest extends MonitoringTestBase {
   +    $this->drupalGet('admin/config/system/monitoring/sensors/add');
   +
   +    // Test creation of File entity aggregator sensor.
   +    $this->drupalPostForm(NULL, array(
   

   Maybe to remove drupalGet() and add the path to drupalPostForm().

6. +++ b/src/Tests/MonitoringUITest.php
   @@ -128,11 +156,83 @@ class MonitoringUITest extends MonitoringTestBase {
   +    $this->assertLink(t('File Entity Aggregator sensor'));
   +    $this->clickLink(t('File Entity Aggregator sensor'));
   

   Same as (1).

+++ b/src/Tests/MonitoringUITest.php
@@ -91,19 +91,24 @@ class MonitoringUITest extends MonitoringTestBase {
-

Revert unnecessary change

+++ b/src/Tests/MonitoringUITest.php
@@ -91,19 +91,24 @@ class MonitoringUITest extends MonitoringTestBase {
+    $this->drupalPostForm('admin/config/system/monitoring/sensors/add', array(

The path is not necessary, we already are on that page. Replace the path with NULL like it was before.

+++ b/src/Tests/MonitoringUITest.php
@@ -974,4 +1061,16 @@ class MonitoringUITest extends MonitoringTestBase {
+  /**
+   * Add verbose fields to the sensor creation form.
+   */
+  public function addFields($fields = array(), $array = array()) {

Missing documentation for both params, and while you are at it, the second parameter could really use a more helpful variable name (it's called $edit most of the time, IIRC)

After that it seems good to go. Adding fields isn't really that elegant, but it works for now.

Nice! Looks good now.

Hm, tests failed because testbot lost dependencies for monitoring again...

Yeah, but we also still have search api and captcha failing even with those. This is postponed on having passing tests again.

I have requested a new alpha release from search api. IMHO it would fix the search_api issue: #2552075: New alpha release
Dunno about the others and if we have issues RTBC / committed?

+++ b/src/Tests/MonitoringUITest.php
@@ -112,15 +118,33 @@ class MonitoringUITest extends MonitoringTestBase {
+    $this->addFields($node_fields, $edit);

@@ -128,11 +152,75 @@ class MonitoringUITest extends MonitoringTestBase {
+    $this->addFields($file_fields, $edit);

I don't like this signature with eating a half prepared $edit and doing the PostForm on its own to NULL.
I would prefer to let it populate $edit with all fields. Name it addAllVerboseFields.

Oh sorry, i just realised that addAllVerboseFields still does and always did some posts on its own to add more items...
I think we should decouple it completely from the other $edit thingies and let it post again on its own.

How about providing some PostFormMultiple(t('Add another field'), 5) that then loop on the button 5 times.
Then adding the values to the $edit is a matter of the caller, followed by the regular Post.
Also, mind that you are using an API to fetch data about base fields. That's indirection. We know what base fields are existing. We can name them in code. You also name them when asserting.

Looks pretty good now!

MonitoringUITest is passing. Rest is unrelated.