Add test coverage

Looks good to me, also drupalci is green
https://www.drupal.org/node/3164537/qa/6370104

1. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerFormPagesTest.php
   @@ -0,0 +1,107 @@
   +  public function testBlockFormAcces(): void {
   

   Access with two s.

2. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerFormPagesTest.php
   @@ -0,0 +1,107 @@
   +    $this->assertBlockFormPageHasAccess();
   +    $this->drupalGet($block_delete_form_path);
   +    $this->assertSession()->pageTextNotContains(t('Access denied'));
   +    $this->assertSession()->pageTextContains($remove_block_label);
   

   I don't like very much these assertBlock<...>Access methods. They wrap just two simple assertions, and here we can see that already they are not useful always.
   Also, instead of asserting the access denied text, assert the response.

3. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerListPagesTest.php
   @@ -0,0 +1,72 @@
   +    $this->assertBlockListPageHasAccess();
   

   Same as above.

4. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerListPagesTest.php
   @@ -0,0 +1,72 @@
   +    $this->assertBlockListPageHasNotAccess();
   

   Let's add a user that has a permission to administer another theme that is not the default one and let's verify that he doesn't get access to this route, since the access check uses the default theme for the permission.

5. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerListPagesTest.php
   @@ -0,0 +1,72 @@
   +    $block_admin_display_theme_path = Url::fromRoute('block.admin_display_theme', [
   

   I would add the above check also here, but it's not mandatory.

6. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerListPagesTest.php
   @@ -0,0 +1,72 @@
   +    // Ensure that user with block_permissions rights has access.
   

   Let's improve this comment: it sounds like there's a global block_permissions permission or similar. Let's state that the user with the permission to administer the stark theme, which is the default theme, has access.

7. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerListPagesTest.php
   @@ -0,0 +1,72 @@
   +    // Ensure that user without block_permissions rights hasn't got access.
   

   Let's adapt also these comments to reflect the previous comment.

8. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerFormPagesTest.php
   @@ -0,0 +1,107 @@
   +   * Tests access to "/admin/structure/block/add/{plugin_id}/{theme}" page.
   
   +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerListPagesTest.php
   @@ -0,0 +1,72 @@
   +      "administer block settings for theme $this->defaultTheme",
   

   We are not testing that the "administer block settings for theme stark" is taken into consideration here. There is actually a bug where you can access and place a block in themes you don't have permissions for. Let's add the coverage and fix also the bug.

9. +++ b/tests/src/Functional/BlockPermissionsAccessControlHandlerFormPagesTest.php
   @@ -0,0 +1,107 @@
   +    $remove_block_label = t('Are you sure you want to remove the block @name?', ['@name' => $page_title_block->label()]);
   

   Let's be extra scrupulous and check that a user with permission to manage blocks from another provider has no access.

10. +++ b/tests/src/Functional/BlockPermissionsBlockLibraryControllerTest.php
    @@ -0,0 +1,58 @@
    +  public function testListBlocks(): void {
    

    This test misses checks to verify that only blocks of the providers where the user has permission can be placed.
    Let's check on the UI that only blocks provided by core are visible (it's enough one) and that blocks provided by another provider (again one is enough) are not.

11. +++ b/tests/src/Functional/BlockPermissionsBrowserTestBase.php
    @@ -0,0 +1,57 @@
    +  /**
    +   * Modules to install.
    +   *
    +   * @var array
    +   */
    

    inheritdoc is enough here.

12. +++ b/tests/src/Functional/BlockPermissionsBrowserTestBase.php
    @@ -0,0 +1,57 @@
    +  protected $adminUser;
    

    Usually admin identifies a user with very high permissions. I'm a bit troubled about moderator and admin, any suggestions?

13. +++ b/tests/src/Functional/BlockPermissionsConfigElementsVisibilityTest.php
    @@ -0,0 +1,67 @@
    +  public function testConfigElementsVisibility(): void {
    

    Let's rename this test. We are not testing the visibility, we are testing the block listing page.

14. +++ b/tests/src/Functional/BlockPermissionsConfigElementsVisibilityTest.php
    @@ -0,0 +1,67 @@
    +    $page_title_block_configuration_selector = "ul[data-drupal-selector=\"edit-blocks-{$page_title_block->id()}-operations\"]";
    +    $page_title_block_weight_selector = "select#edit-blocks-{$page_title_block->id()}-weight";
    +    $page_title_block_draggable_selector = "tr.draggable[data-drupal-selector=\"edit-blocks-{$page_title_block->id()}\"]";
    +    $page_title_block_undraggable_selector = "tr.undraggable[data-drupal-selector=\"edit-blocks-{$page_title_block->id()}\"]";
    

    We can simplify here. Let's fetch the wanted table row by data-drupal-selector as done here, and then get the remaining elements from there with $row->find().
    Also instead of checking that a ul is present, we can check that the configure link is present or not in the specific rows.

15. +++ b/tests/src/Functional/BlockPermissionsConfigElementsVisibilityTest.php
    @@ -0,0 +1,67 @@
    +    $this->assertSession()->elementNotExists('css', $page_title_block_undraggable_selector);
    

    Let's check here also that we can't edit blocks with a different provider.

16. I think that the test file names are too long. Let's remove the module name from the file/class.
    For example BlockPermissionsAccessControlHandlerFormPagesTest could be BlockFormRoutesAccessTest.

unfortunately links like https://www.drupal.org/node/3164537/qa/6370104 are not valid anymore so I cant check if this is ok on drupalci, so I ran these on local with the mentioned patches added:

• https://www.drupal.org/project/block_permissions/issues/3168737
• 173321 (https://www.drupal.org/project/block_permissions/issues/3164744)

fails for me at

There was 1 error:

1) Drupal\Tests\block_permissions\Functional\BlockFormRoutesAccessTest::testBlockAddFormAccess
Behat\Mink\Exception\ExpectationException: Current response status code is 200, but 403 expected.

/Users/andrasszilagyi/Projects/Drupal/vendor/behat/mink/src/WebAssert.php:768
/Users/andrasszilagyi/Projects/Drupal/vendor/behat/mink/src/WebAssert.php:130
/Users/andrasszilagyi/Projects/Drupal/web/modules/contrib/block_permissions/tests/src/Functional/BlockFormRoutesAccessTest.php:79

Maybe we should not make this dependent on the other issues, or link the dependent issues and mention in the ticket description that those need to be resolved first.

Also a interdiff would be nice, a merged patch would not hurt.

Other then that the code style look very good to me and the approach is correct.

ok for me