Blocks operations cannot be altered

oops a bit of confusion between the similar issues.

noticed some things while working on this and menu issue.
build -> built
also the hook function needs to use *this* module name,
and we need to enable the test module so the hook has effect.

we expect this to fail, because of #2

ready for review.

forgot the patch.

@tim.plunkett Thanks. OK. dont use hook in a name. I'll remember. :)

Don't we only "use" something if it's used more than once?
Maybe I'm reading this wrong:

Note that if a class is used more than once in multiple hook signatures, it must still be "use"ed, and then only the short names of the class should be used in the function.

https://drupal.org/node/1353118

Should we move the test module to the entity module?

Also, I've only made issues to add tests for menu, views, blocks..

Can't it just enable all of the config entities and test them?

are you suggesting we duplicate all the tests from config_translation?

http://drupalcode.org/project/config_translation.git/tree/HEAD:/lib/Drup...
Most are in one file, but views is in separate.
http://drupalcode.org/project/config_translation.git/blob/HEAD:/lib/Drup...
http://drupalcode.org/project/config_translation.git/blob/HEAD:/lib/Drup...

I'll test this and review it now. :)

A. The fix looks good.

B. This looks really great. :) Good location for the tests.

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
   @@ -32,11 +48,23 @@ public static function getInfo() {
   -    // Create and login user.
   -    $this->web_user = $this->drupalCreateUser(array(
   +    $permissions = array(
   +      'access site-wide contact form',
   +      'administer blocks',
   +      'administer contact forms',
   +      'administer filters',
   +      'administer menu',
          'administer permissions',
   -    ));
   -    $this->drupalLogin($this->web_user);
   +      'administer shortcuts',
   +      'administer site configuration',
   +      'administer taxonomy',
   +      'administer users',
   +      'administer views',
   +    );
   +
   +    // Create and log in user.
   +    $admin_user = $this->drupalCreateUser($permissions);
   +    $this->drupalLogin($admin_user);
   

   Is changing from a web_user with fewer permissions to an admin user with more permissions removing some test that was here before or changing what it was testing before?

   Answering my own question:
   oh, I think this test was only doing the foreach role on the roles page, so it's ok here to just use the admin user like it is.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
   @@ -45,14 +73,191 @@ public function setUp() {
   +   * Tests custom block lists to see if test_operation link is added to operations.
   ...
   +   * Tests contact forms lists to see if test_operation link is added to operations.
   

   changing the comment to refer to test_operation pushes this over 80 chars.

   And in some other places.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
   @@ -45,14 +73,191 @@ public function setUp() {
   +    // Create a test role to decouple looking for test_operation operations
   +    // link so this does not test more than necessary.
   +    $role_id = Unicode::strtolower($this->randomName(16));
   +    $this->drupalCreateRole(array(), $role_id);
   +
   +    // Get the role listing.
        $this->drupalGet('admin/people/roles');
   -    $roles = user_roles();
   -    foreach ($roles as $role) {
   -      $uri = $role->uri();
   -      $this->assertLinkByHref($uri['path'] . '/test_operation');
   -      $this->assertLink(format_string('Test Operation: @label', array('@label' => $role->label())));
   -    }
   +
   +    $test_operation_link = 'admin/people/roles/manage/' . $role_id . '/test_operation';
   +    // Test if the link to test_operation the role is on the page.
   +    $this->assertLinkByHref($test_operation_link);
   

   Mmm. The old test here was looking for the test_operation of "each" role. But we are not sure there were any.

   The new test is just looking for the link on one role.

   Is it worth merging the approaches? Making a test role, and then *also* doing a foreach?

   I could see some strange bug one day that might do something like only the first item in the list gets the operation added.

   Maybe we should make a few test things, and do the foreach. Is it worth it to do that? (I did not do that.)

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
   @@ -45,14 +73,191 @@ public function setUp() {
   +    // Test if the link to test_operation the vocabulary is on the page.
   

   heh. This used to make sense when it was "Test if the link to translation the vocab..." Fixed.

   +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
   @@ -45,14 +73,191 @@ public function setUp() {
   +    // Views UI List 'admin/structure/views'.
   

   This is I think left over from when we had all the tests in one giant function and we had section headers. Also, it's not a sentence. I think with the nice functions this is not needed anymore. Taking it out.

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityOperationsTest.phpundefined
   @@ -7,19 +7,35 @@
   +   * @var array
   +   */
   +  public static $testViews = array('test_view');
   +  /**
       * Modules to enable.
   

   missing newline. added it.

oh, also when the method for the views got pasted in, the newline before the last } on the class was lost. I put the newline back in.

updated the issue summary and marked #2023739: menu: hook_entity_operation_alter() does not work with EntityListController as a duplicate of this issue.

OK.
So we will just do the test for blocks here, (and not use the doPattern here).

We will make the do... tests protected in config_translation where we use this same pattern.

Regarding

I know that this is a duplicate test... there is a high chance we are introducing duplicate tests here.

We talked about this, and since it is testing the addition of a link added by the alter, it's not actually a duplicate. :)

I'll work on this.
--------
After this issue, we should make a generic test that can be extended instead of putting a separate independent test in each place.
Once we do that, then we can use *that* in config_translation also.

I went back to #5 (aka 6). And redid the fixes asked for after that.

updating title as we have changed back to original strategy of just testing blocks.
I'll update the issue summary.

Updated issue summary.

added the issue to make tests generic

added some fancy 'concern of' words.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

#21: drupal8.block-module.2027857-21.patch queued for re-testing.

updating comment number that it's updated as.

#21: drupal8.block-module.2027857-21.patch queued for re-testing.

This does not apply anymore.
Maybe because #1981144: Convert block_admin_edit() in block.admin.inc to the new form interface got it.

tag!