Add additional testing coverage for Olivero

Are there any examples we can point to in other themes of the type of tests we are looking to create?

I reviewed the other themes in core and I am not seeing examples of tests. Maybe they live elsewhere in the codebase?

Started looking at this during the FLDC sprint, but admittedly didn't get too far. Planning on continuing to chip away at this one.

Pushed a commit up to the issue fork that adds coverage for olivero_preprocess_block

I changed the tests to use Olivero as a default theme, rather than manually enabling it. One side effect of that is that I also needed to use the minimal install profile during the set up phase. With other approaches I kept seeing unmet dependency messages, even when adding the dependencies during setup. Could be an opportunity to optimize these tests.

I also couldn't think of a way to cover this:

if ($variables['base_plugin_id'] === 'search_form_block' && in_array($region, ['primary_menu', 'secondary_menu'], TRUE)) {
  $variables['content']['actions']['submit']['#theme_wrappers'] = ['input__submit__header_search'];
}

Isn't clear to me what the impact that this has on the resulting page.

Had previously discussed with @mherchel about treating this as a meta issue and creating individual issues for the line items listed in the summary. I realized that would be difficult to manage since additional tests will want the set up changes in this merge request. So I think for the MidCamp sprint we're better off coordinating and all working off of the existing feature branch.

I did create #3205434: Add Nightwatch Test Coverage for Olivero for Nightwatch tests, which can be addressed separately.

I'm taking olivero_form_alter

I'm going to pick up preprocess_input_test from, @mtift here at midcamp 2021.

Preprocess input test is done pending review.

I am working on writing the test for number 6 as part of MidCamp 21 contribution day.

Great progress on this issue at the MidCamp sprint today. We now have tests for the first 5 theme functions, and as noted above coverage for the last function is in progress.

The test I wrote for olivero_preprocess_block is a phpunit functional test, while everything written today are unit tests. Not sure if we want to change that. There are a few things covered in there that seem like they might be a little harder to handle in a unit test.

I am done for the day, thank you @mradcliffe for mentoring and helping me today.
This is my first contribution to the Drupal project. Hooray!!!

The previous comment doesn't include a test for the portion of the preprocess that adds user.compact because it doesn't seem to be executing. Can anyone clarify if this actually is necessary? If not, should it be removed? Otherwise, it still needs a test.

At this point, I think we just need one last holistic review to make sure all of the tests still pass, and we didn't miss something coverage wise. Would be a good issue for someone with testing experience looking to pick something up during DrupalCon contribution.

I'm not sure about the answers to the "unresolved" issues above, but I cleaned up a few minor coding standard things and confirmed that all of the tests pass. This looks good to me.

@mradcliffe mentioned on slack that he thought olivero_preprocess_input might be missing some coverage. Taking a quick look at the code, this block is the only thing that looks like it might not be covered.

  if (in_array($type_html, $text_types_html, TRUE)) {
    $variables['attributes']['class'][] = 'form-element';
    $variables['attributes']['class'][] = Html::getClass('form-element--type-' . $type_html);
    $variables['attributes']['class'][] = Html::getClass('form-element--api-' . $type_api);

    if (!empty($variables['element']['#autocomplete_route_name'])) {
      $variables['autocomplete_message'] = t('Loading…');
    }
  }

  if (in_array($type_html, ['checkbox', 'radio'], TRUE)) {
    $variables['attributes']['class'][] = 'form-boolean';
    $variables['attributes']['class'][] = Html::getClass('form-boolean--type-' . $type_html);
  }

I think we're good on the form-element lines. I don't think we have something covering $variables['autocomplete_message'] or the form-boolean--type section for checkbox and radio.

Between that and @mtift's review and updates I can't think of anything that would still be missing here.

Added coverage as outlined in #32. Also removed the WIP status from the MR. As far as I'm concerned this is complete and ready for the real deal review.

All of the tests pass, the code coverage seems sufficient, and the test code makes sense to me.

I'd have hoped that the test coverage would pick up this...

  if (isset($form['actions']) && (count($form['actions'])) <= 2) {
    $form['actions']['submit']['#attributes']['class'][] = 'button--primary';
  }

I think this should be if (isset($form['actions']['submit']) && (count($form['actions'])) <= 2) {. It's possible to have actions without a submit - here's an example - http://grep.xnddx.ru/node/30840640 - this contrib module will do interesting things with olivero enabled.

I think we should consider whether unit tests of form alters and preprocess functions are the best way to go. The tests are deeply coupled to the implementations. If the underlying form altering mechanism were to change what we'd want is a test that proves the effects are still working.

> I think we should consider whether unit tests of form alters and preprocess functions are the best way to go. The tests are deeply coupled to the implementations. If the underlying form altering mechanism were to change what we'd want is a test that proves the effects are still working.

Had some follow up discussion in slack and clarified that functional tests are the more appropriate way to cover the hooks we're testing here. So the work required here would be refactoring the unit tests added here to be functional tests. I believe olivero_preprocess_block was initially covered as a functional test, so that should be set.

We should also address the specific case identified in #36 along the way.

Performing some late night hacking on this.

Made a Nightwatch test to cover the form alters for search. I started on something for comments, but it's a bear to setup in Nightwatch and should be a BrowserTestBase – the main goal is testing the comment count displayed as part of OliveroPreprocessFieldCommentTest

olivero_form_alter is almost covered. We need functional tests to convert the Unit tests for all of the button class manipulation.

Claiming olivero_preprocess_field__comment :)

Pushed up coverage for olivero_preprocess_input. It doesn't cover datetime-local, month, and week input types - there were no examples in the form_test module (or anywhere else in core that I could find) and I wasn't able to easily create examples. I'm hoping the fact that this does cover date and time inputs is good enough.

This also does not cover the rendering of an autocomplete loading message

    if (!empty($variables['element']['#autocomplete_route_name'])) {
      $variables['autocomplete_message'] = t('Loading…');
    }

It looks like there isn't anything in the theme that actually renders this message. Planning on creating a follow up issue for this.

Next step for me is that I'm going to walk through all the tests again, provide some review and close any remaining gaps that I can.

Reviewed Nightwatch coverage for:
* olivero_preprocess_block - this is a BrowserTestBase test, but is a functional test that passes. Planning on creating a follow up ticket to convert this to Nightwatch, and hoping that the existing test coverage is good enough for stable.
* olivero_theme_suggestions_form_alter - confirmed that it is covered by oliveroSearchFormTest.js
* olivero_form_alter - reviewed the existing test and added remaining coverage for the submit button class.

That just leaves olivero_preprocess_field__comment. Similar to form_alter I'm planning on next reviewing the existing test and trying to fill in any coverage gaps if necessary.

Pushed updates to oliveroComentTest.js that should cover what remains for comments.

From my perspective all that remains at this point is resolving the issue related to $form['actions'] referenced in https://www.drupal.org/project/drupal/issues/3174107#comment-14091227 and adding test coverage for it.

From my perspective all of the unit tests are now replaced by Nightwatch tests and this is ready to be reviewed again. olivero_preprocess_block is still covered by a BrowserTestBase test. I'll create a follow up issue for that as it would be nice to have all tests in one place at this point.

You can run all of Olivero's tests with:

phpunit --group olivero
nightwatch --tag olivero

@mherchel It looks like this re-queued somehow and everything passed.

@mherchel thanks for the heads up. Rebased against 9.2.x to resolve the merge conflict.

Updated the variable name as suggested and confirmed locally that the Nightwatch tests still pass.

Looks like some of the selectors changed since the search test was added. Side effect of code standards cleanup perhaps?

Pushed up updates for the failing test and ran the Olivero suite locally to confirm that everything passes.

I think this is ready to go. Great work, @brianperry!

There's a tiny thing to update in the .theme code changes - after that this looks really good.

Made changes as per comment #58.

The latest change looks good. Back to RTBC.

PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.13
Configuration: /Users/alex/dev/sites/drupal8alt.dev/phpunit.xml

Testing Drupal\Tests\config\Functional\ConfigImportAllTest
E                                                                   1 / 1 (100%)

Time: 47.26 seconds, Memory: 8.00 MB

There was 1 error:

1) Drupal\Tests\config\Functional\ConfigImportAllTest::testInstallUninstall
Drupal\search\Exception\SearchIndexException: Failed to clear index for type 'help_search', sid '7' and langcode ''

/Users/alex/dev/sites/drupal8alt.dev/core/modules/search/src/SearchIndex.php:253
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:507
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:438
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:459
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/HelpSectionManager.php:45
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Plugin/CachedDiscoveryClearer.php:31
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/ProxyClass/Plugin/CachedDiscoveryClearer.php:83
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Extension/ModuleInstaller.php:508
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:91
/Users/alex/dev/sites/drupal8alt.dev/core/modules/config/tests/src/Functional/ConfigImportAllTest.php:111
/Users/alex/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

Caused by
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8alt.test14886880search_index' doesn't exist: DELETE FROM "test14886880search_index"
WHERE ("type" = :db_condition_placeholder_0) AND ("sid" = :db_condition_placeholder_1); Array
(
    [:db_condition_placeholder_0] => help_search
    [:db_condition_placeholder_1] => 7
)


/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Driver/mysql/ExceptionHandler.php:53
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Query/Delete.php:61
/Users/alex/dev/sites/drupal8alt.dev/core/modules/search/src/SearchIndex.php:249
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:507
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:438
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:459
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/HelpSectionManager.php:45
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Plugin/CachedDiscoveryClearer.php:31
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/ProxyClass/Plugin/CachedDiscoveryClearer.php:83
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Extension/ModuleInstaller.php:508
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:91
/Users/alex/dev/sites/drupal8alt.dev/core/modules/config/tests/src/Functional/ConfigImportAllTest.php:111
/Users/alex/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

Caused by
PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8alt.test14886880search_index' doesn't exist

/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:145
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Query/Delete.php:57
/Users/alex/dev/sites/drupal8alt.dev/core/modules/search/src/SearchIndex.php:249
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:507
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:438
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/Plugin/Search/HelpSearch.php:459
/Users/alex/dev/sites/drupal8alt.dev/core/modules/help_topics/src/HelpSectionManager.php:45
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Plugin/CachedDiscoveryClearer.php:31
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/ProxyClass/Plugin/CachedDiscoveryClearer.php:83
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Extension/ModuleInstaller.php:508
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:91
/Users/alex/dev/sites/drupal8alt.dev/core/modules/config/tests/src/Functional/ConfigImportAllTest.php:111
/Users/alex/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

Unfortunately this test fail is caused by this MR for some reason.

I've pushed a fix to the branch but unfortunately this is a completely unrelated issue and is only happening because of the dependencies added to olivero_test. I've created #3218660: help_topics module can break during module uninstall to fix and add a proper test for the fix.

Updated the MR with the proper fix from #3218660: help_topics module can break during module uninstall - it'll prove that the fix works but, unfortunately, I think that that one is going to have to land first :(

Took another look at the MR, whilst I don't particularly like using nightwatch, if that's what people are comfortable with, so be it.

I was at first questioning a lot of the changes as out of scope, but after reading the comments realise this includes #3218660: help_topics module can break during module uninstall which we should postpone this issue on.

I'll take a look at that now.

+1, the changes look good since we moved from unit tests to functional tests. I believe the Nightwatch tests work to cover interactions and modifications.

Committed e182007 and pushed to 9.3.x. Thanks!

this commit appears to have caused automated testing to fail for php 8.0.

PHPUnit Test failed to complete; Error: PHPUnit 9.5.6 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\FunctionalTests\Theme\OliveroTest
..E.                                                                4 / 4 (100%)

Time: 00:37.741, Memory: 4.00 MB

There was 1 error:

1) Drupal\FunctionalTests\Theme\OliveroTest::testPreprocessBlock
Behat\Mink\Exception\ElementNotFoundException: Element matching css "ul[region="primary_menu"]" not found.

https://www.drupal.org/pift-ci-job/2107053

this is holding up test jobs from other issues

I've created a new MR against 9.3.x because one of the new tests is failing there...

I'm going to flip this back to RTBC. Removed the failing assertion which isn't true in the 9.3.x branch

2nd time lucky! Committed e595a45 and pushed to 9.3.x. Thanks!