Implement testing

Thanks a lot for working on this! I have enabled automatic testing and ran the test. The result is green, but it generates a number of coding standards errors:

tests/src/Functional/SiteAlertMenuTest.php
line 3	There must be one blank line after the namespace declaration
13	Missing member variable doc comment
48	Doc comment short description must end with a full stop
61	Missing function doc comment
76	Missing function doc comment
77	Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
92	Missing function doc comment
93	Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
96	Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
105	Expected 1 blank line after function; 0 found
106	Expected 1 newline at end of file; 2 found
106	The closing brace for the class must have an empty line before it

I have not yet reviewed the test manually, but let's already fix these before continuing.

Reviewed the patch from #5. All coding issues are fixed.

I reviewed the patch and there are few things that stand out:

1. Incorrect group annotation

/**
 * Tests that the Site Alert is working correctly.
 *
 * @group admin_toolbar
 */

2. There are four tests which means that four times a drupal installation and teardown is performed which slows the execution of tests. All four tests could be merged into a single test with assertions in correct order:

- Alerts list assertions
- Alert creation assertions
- Block placement assertions
- Alert deletion assertions
- No block after delete assertions

3. Previous point would also fix the next problem in tests, eg.

  /**
   * {@inheritdoc}
   */
  public function testDeleteAlert() {
    // Creates alert.
    $this->testCreateAlert();
    ...
  }

Tests are calling another test that already ran and succeeded/failed.

One more thing I found, probably just nitpicking here but lets follow the standards. Each module in the list should be in separate line.

  /**
   * {@inheritdoc}
   */
  protected static $modules = [
    'site_alert', 'options', 'block',
  ];

Awesome, thanks so much for working on this! Assigning for review.

1. +class SiteAlertMenuTest extends SiteAlertTestBase {
   

   We're not testing any menu's so the name of the test is a bit misleading. Since this is testing the adding and removing of an alert through the web interface a better name would be SiteAlertUiTest.

+  protected static $modules = [
+    'site_alert',
+    'options',
+    'block',
+  ];

It is no longer needed to explicitly add the dependency on the options module, this has been fixed recently in #3111058: Missing dependency on the Options module.

+  /**
+   * A test user with permission to access the administrative toolbar.
+   *
+   * @var \Drupal\user\UserInterface
+   */
+  protected $adminUser;

This user doesn't have the permission to access the toolbar, so let's change this to A test user with permission to administer site alerts..

3. +  /**
   +   * Tests that the site alter list page works.
   +   */
   

   This documentation seems to be unrelated to what is being tested, we are not altering any pages in the test. Let's put a description here that explains what this is about, maybe something like Tests the creation and deletion of site alerts through the user interface.

4. +    $this->assertSession()
   +      ->responseContains('<div id="block-' . $block_id . '">');
   

   We don't need to call $this-assertSession() again, it has already been called before and stored in the $assert variable. Also it is not needed to check the existence of actual HTML snippets since this is brittle; any change in the theming (like adding a class) would break this test. Instead we can use WebAssert::elementExists() to check if the block is on the page.

5. +    /** @var \Drupal\Tests\WebAssert $assert */
   +    $assert->pageTextContains('This action cannot be undone.');
   

   There are a few cases where this inline declaration of the variable type is left over in the code from earlier iterations. We can remove the duplicates, it is sufficient to keep only the declaration at the moment the variable is instantiated.

6. +    // Test that there is an empty reaction rule listing.
   +    $assert->pageTextContains('There are no site alert entities yet.');
   

   The comment from this seems to be unrelated, possibly copied from a different test? Also let's add a test to check that this empty text is no longer visible once we created a site alert.

Crossposted with comment #15. Some of my remarks were already addressed, great :)

Here are the rest of the fixes. This will be good to go when tests come back green. I would like to get this in first before any other issues so that we can run the test and check that there are no regressions.

Thanks a lot!