Contributor tasks needed
Task Novice task? Contributor instructions Complete?

As per the parent issue.

./Theme/EngineTwigTest.php
./Theme/EntityFilteringThemeTest.php
./Theme/FunctionsTest.php
./Theme/HtmlAttributesTest.php
./Theme/ThemeSuggestionsAlterTest.php
./Theme/ThemeTest.php
./Theme/TwigDebugMarkupTest.php
./Theme/TwigFilterTest.php
./Theme/TwigNamespaceTest.php
./Theme/TwigRawTest.php
./Theme/TwigTransTest.php

Lots of these can be kernel tests of split into browser and kernel tests.

CommentFileSizeAuthor
#20 interdiff-2863429-18-20.txt12.81 KBApacheEx
#20 2863429-20.patch29.01 KBApacheEx
#18 2863429-18.patch40.21 KBLendude
#18 interdiff-2863429-16-18.txt4.33 KBLendude
#16 2863429-16.patch39.57 KBLendude
#16 interdiff-2863429-14-16.txt17.86 KBLendude
#14 2863429-14.patch23.01 KBLendude
#2 2863429-2.patch10.12 KBjofitz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jo Fitzgerald created an issue. See original summary.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Assigned: jofitz » Unassigned
Status: Active » Needs review
FileSize
2863429-2.patch10.12 KB
  • Moved all 11 files.
  • Corrected namespace.
  • (no references to any of these classes elsewhere).
jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Component: cache system » theme system

Corrected the component.

Status: Needs review » Needs work

The last submitted patch, 2: 2863429-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

We could wait until the trait is ported on #2863318: Cache: Convert system functional tests to phpunit ... on the other hand I kinda believe we should move the trait directly.

michielnugter’s picture

michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented
Issue tags: +phpunit initiative

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented
Assigned: Unassigned » mikejw
mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented

Have created a related issue: https://www.drupal.org/node/2912776 that this depends on.

mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented

So methods called from AssertContentTrait that are used:
- assertThemeOutput, which calls
- assertIdentical
- setRawContent

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented

@mikejw If setRawContent is needed, then that part of the test needs to be converted to a KernelTest.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs work » Needs review
FileSize
2863429-14.patch23.01 KB

Some conversions to kernel tests, some other stuff. Not everything will work yet. No interdiff cause #2 failed to apply so needed a small reroll.

Status: Needs review » Needs work

The last submitted patch, 14: 2863429-14.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
interdiff-2863429-14-16.txt17.86 KB
2863429-16.patch39.57 KB

This was running pretty smooth locally, lets see if Mr. Bot agrees.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs review » Needs work

Mr. Bot does agree, but I don't yet - sorry @Lendude!

  1. +++ b/core/modules/system/tests/src/Functional/Theme/HtmlAttributesTest.php
@@ -23,9 +23,8 @@ class HtmlAttributesTest extends WebTestBase {
-    $attributes = $this->xpath('/html[@theme_test_html_attribute="theme test html attribute value"]');
-    $this->assertTrue(count($attributes) == 1, "Attribute set in the 'html' element via hook_preprocess_HOOK() found.");
...
+    $this->assertSession()->responseContains('<html lang="en" dir="ltr" theme_test_html_attribute="theme test html attribute value">');

    This looks like it's gotten more fragile? It does look like it's testing the same thing and more clearly - so probably not more fragile, but more explicit. Nevermind - good change! 

  2. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeSuggestionsAlterTest.php
@@ -170,16 +170,20 @@ public function testExecutionOrder() {
-    $expected = [
+    $expected_order = [
+      'theme_test_theme_suggestions_alter() executed.',
       'theme_suggestions_test_theme_suggestions_alter() executed.',
       'theme_suggestions_test_theme_suggestions_theme_test_suggestions_alter() executed.',
-      'theme_test_theme_suggestions_alter() executed.',
       'theme_test_theme_suggestions_theme_test_suggestions_alter() executed.',

    huh? This looks like this is actually a changed test? Was this not executing earlier or how is this possible? 

  3. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeTest.php
@@ -0,0 +1,190 @@
+   * Modules to enable.
+   *
+   * @var array

    {@inheritdoc} 

  4. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeTest.php
@@ -0,0 +1,190 @@
+  protected function setUp() {

    Needs docblock 

  5. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeTest.php
@@ -0,0 +1,190 @@
+    for ($i = 0; $i < 2; $i++) {
+      $this->drupalGet('theme-test/suggestion');

    Doing this with a for-loop is weird, it's only the same amount of lines, but less clear. Let's change this. 

  6. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeTest.php
@@ -0,0 +1,190 @@
+    // Ensure that the custom theme negotiator was not able to set the theme.
+
+    $this->assertNoText('Theme hook implementor=test_theme_theme_test__suggestion(). Foo=template_preprocess_theme_test', 'Theme hook suggestion ran with data available from a preprocess function for the base hook.');

    There should not be an empty line following an inline coment. 

  7. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeTest.php
@@ -0,0 +1,190 @@
+  public function testClassLoading() {
+    new ThemeClass();
+  }

    This should probably not be in seperate method in a browsertest? Can we move this to a kernel test instead? 

  8. +++ b/core/modules/system/tests/src/Kernel/Theme/TwigNamespaceTest.php
@@ -1,15 +1,14 @@
+use Drupal\KernelTests\KernelTestBase;
 /**

    Looks like this use is hugging the docblock, they need to be 1 tile apart!

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs work » Needs review
FileSize
interdiff-2863429-16-18.txt4.33 KB
2863429-18.patch40.21 KB

@borisson_ thanks so much for the reviews!

#17.2 yeah sorta surprised me too. Dug a little deeper and turns out that hook is called like 8 times and one was in the right order but the rewrite was testing for the first instance. Updated the message to make this much clearer but might effect other tests if this is used in other places.
#17.5 this is actually the original webtestbase test, so I'd say we just leave it as is. It's not pretty but out of scope.
#17.7 yeah I missed that when I spit up the test, nice catch!

Fixed the rest. Some of it might be a bit out of scope, but the patch is hard to read for ThemeTest, some of it's new, some of it's old, so hard to see which changes are in scope for a conversion.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

Thanks for fixing/answering my remarks. Looks super solid.

ApacheEx’s picture

ApacheEx CreditAttribution: ApacheEx as a volunteer and at Internetdevels, Drupal Ukraine Community commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
2863429-20.patch29.01 KB
interdiff-2863429-18-20.txt12.81 KB

Just a few improvements from my side:

1) TwigFilterTest moved to KernelTestBase
2) Reverted FunctionsTest to almost original version, just need to install classy theme as default to use right templates.
3) Reverted position of ThemeTest::testClassLoading, and added blank line.

So, -10KB to original patch :)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Assigned: mikejw » Unassigned
Status: Needs review » Reviewed & tested by the community

Great work @ApacheEx! That looks like a great improvement.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

  • catch committed e8c9956 on 8.7.x 
    Issue #2863429 by Lendude, ApacheEx, Jo Fitzgerald, borisson_: Theme:...

  • catch committed 194c1ff on 8.6.x 
    Issue #2863429 by Lendude, ApacheEx, Jo Fitzgerald, borisson_: Theme:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.