Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, test.theme_.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

I almost feared that.

But that said - the failing assertions in that test are actually completely unnecessary:

- The test verifies that local CSS files can be overridden by modules/themes, using the same basename of a CSS file.

- It performs a first round of system.base.css being overridden by another test dummy file. (which still passes with this patch)

- Then it performs a second round in the reverse order. This fails, because the test does not call drupal_static_reset('drupal_add_css') to clear out the existing added CSS files first, which means that the files from the first round are still registered (i.e., you register file 1, file 2, then file 2, file 1). The algorithm for applying CSS overrides in drupal_get_css(), however, simply says: "whoever added it last, wins." -- since drupal_add_css() still has the first two, and because neither their array keys nor their weights have changed, file 2 still wins.

I actually don't know why this failure is suddenly exposed with this patch, since it seems completely unrelated to the theme functionality...

Anyway, this patch blocks #1067408: Themes do not have an installation status, so it would be great to get it in ASAP.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.phpundefined
@@ -215,14 +215,6 @@ function testRenderOverride() {
     $this->assert(strpos($styles, $system . '/system.base.css') === FALSE, t('The overridden CSS file is not output.'));
-
-    drupal_add_css($system . '/tests/system.base.css');
-    drupal_add_css($system . '/system.base.css');
-
-    // The standard stylesheet should be the only one included.
-    $styles = drupal_get_css();
-    $this->assert(strpos($styles, $system . '/system.base.css') !== FALSE, t('The overriding CSS file is output.'));
-    $this->assert(strpos($styles, $system . '/tests/system.base.css') === FALSE, t('The overridden CSS file is not output.'));

Removing tests isn't the best solution.

Perhaps #1388546: Adding CSS or JS files twice changes weight would help?

sun’s picture

Status: Needs work » Needs review

Well, I hoped I had provided sufficient details for why these assertions are plain simply needless. Trying once more:

They do not test any different functionality. The removed lines are exactly identical to the previous lines, just with a reversed order. The CSS processing does not care in which order the files are - as long as the basename is the same, the result is identical.

#1388546: Adding CSS or JS files twice changes weight is not relevant (- that said, the weights did not actually change in the verbose debug output, so I wonder whether that issue might be obsolete - anyway, OT)

sun’s picture

FileSize
2.31 KB

But then again, I have better things to do than to fight over completely needless test code assertions ;)

tim.plunkett’s picture

Contrary to #4, combining the patch from #1388546-24: Adding CSS or JS files twice changes weight and the patch in the OP, no resets or removals, does pass.

sun’s picture

Huh. Interesting. I'm sorry for dismissing the relevance, and thanks for proving me wrong, @tim.plunkett! :)

So I guess the proper procedure would be to get #1388546: Adding CSS or JS files twice changes weight into core first, in order to unblock this patch... That said, I'm not sure whether the entirety of those weight changes make sense; I'll comment on the other issue.

sun’s picture

FileSize
1.72 KB

Alrighty: Further debugging and testing on #1388546: Adding CSS or JS files twice changes weight revealed the actual cause for the test failure in that CSS test:

  1. It asserts that the CSS being output is identical in consecutive calls to drupal_get_css().
  2. However, drupal_get_css() calls into drupal_render(), which calls into theme(), which "lazy-initializes" the theme if it hasn't been initialized yet.
  3. Thus, the test fails, because the output is no longer identical after the first call to drupal_get_css().

Thus, I've added a slight adjustment for this in attached patch to make sure that the theme being used by tests is correctly initialized within the test already.

sun’s picture

Passed as expected.

Would love to quickly move forward here, since this patch is the primary blocker for #1067408: Themes do not have an installation status

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

That's a great fix.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Seems consistent with what we do elsewhere for this kind of situation.

Committed and pushed to 8.x. Thanks!

Moving to 7.x for backport.

sun’s picture

Priority: Major » Normal

Thanks!

Decreasing priority for D7, since nothing is blocked there. Also, a backport would be sorta blocked on the backport of #1563620: All unit tests blow up with a fatal error (which brings the code-base in line).

sun’s picture

Assigned: sun » Unassigned
sun’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs review
FileSize
2.27 KB

Ahem, *cough* ;)

Turns out that my patch in #1067408: Themes do not have an installation status still throws exceptions despite this fix.

The reason for that is that the theme is re-initialized too early in TestBase::prepareEnvironment(), which means that the global theme is unset, but immediately after re-set to the very same thing - D'OH! :P

Attached patch moves the re-initialization into the ::setUp() methods of the base classes - after the database prefix has been changed (and in case of WebTestBase, after the new site has been installed), which makes a lot more sense ;)

sun’s picture

hah, seriously did not expect that patch to pass on the very first shot, great. ;)

Ehhh, scratch that -- the comment was meant for another issue ;)

sun’s picture

Status: Needs review » Reviewed & tested by the community

This is just a very trivial follow-up to the original patch, so I hope you don't mind if I RTBC my own patch.

sun’s picture

Ping?

sun’s picture

#14: drupal8.test-theme.14.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.test-theme.14.patch, failed testing.

tim.plunkett’s picture

sun’s picture

#14: drupal8.test-theme.14.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7, +Testing system

The last submitted patch, drupal8.test-theme.14.patch, failed testing.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
2.19 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-theme.23.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.33 KB

Geez. No idea how many more times I need to fix that bug.

The additional system_list() fix is contained in ~5 different branches/patches in the meantime.

sun’s picture

FileSize
2.19 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-theme.26.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Oops, please ignore #26 - I wrongly assumed the system_list() fix was contained in #1810990: Teared down test environment leaks into parent site/test runner - but it is not.

#25 is ready.

sun’s picture

#25: drupal8.test-theme.25.patch queued for re-testing.

sun’s picture

#26: drupal8.test-theme.26.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-theme.26.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review

#25: drupal8.test-theme.25.patch queued for re-testing.

mbrett5062’s picture

#26: drupal8.test-theme.26.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7, +Testing system

The last submitted patch, drupal8.test-theme.26.patch, failed testing.

mbrett5062’s picture

OK that last test run dramatically improved test results, but nothing changed in patch. Testing manually to see if I can figure this out.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
2.18 KB

@sun, I hope you don't mind me working on this, I realize it is assigned to you, but it seems you have not looked at it in a while, and you may have good reason. I could be wasting my time, but anyway.

It is just that I have found the solution to the test failures, I believe. So here is a new patch for testing. I think the reason is a change in unit tests. With the addition of DrupalUnitTestBase, once I moved the code to initialize the theme into there from UnitTestBase, the errors went away locally.

Anyway here is the revised patch, lets see what the test bot says.

mbrett5062’s picture

OK not all tests passing locally, but some that failed previously, do, maybe I have not moved code into the correct place, will investigate further after xmas. Have a good one people.

mbrett5062’s picture

hhmmmm, and they pass here. That's weird, but cool.

chx’s picture

So how do we test this...?

chx’s picture

Also, I am not sure whether it's correct to run theme init -- maybe? Why not just unset the globals?

mbrett5062’s picture

Sorry @chx, I can not answer your questions, really only jumped in to re-roll/fix test failures. The underlying architecture is beyond me. Why @sun did what he did and how to test I have no idea. Sorry if that is unhelpful, hopefully @sun will pipe in. Also see #14 for an explanation of the 'why?'.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7, +Testing system

The last submitted patch, drupal-test_theme-1770902-36.patch, failed testing.

penyaskito’s picture

Issue tags: +Needs reroll
InternetDevels’s picture

sun’s picture

Status: Needs review » Fixed
Issue tags: -Needs backport to D7

I don't know which issue/patch fixed it, but the offending call to drupal_theme_initialize() no longer exists.

Thus, marking as fixed. I do not feel comfortable with backporting this change, because it changes the test environment, which in turn may cause previously passing tests to suddenly fail.

Status: Fixed » Closed (fixed)

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

sun’s picture

Status: Closed (fixed) » Needs work
sun’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

Re-opened for #1067408: Themes do not have an installation status

Gotcha. The theme data cannot be taken over from the test runner, because it is already post-processed, based on the currently enabled extensions.

sun’s picture

Priority: Normal » Major
Parent issue: » #1067408: Themes do not have an installation status
FileSize
1.82 KB

Further work on that issue revealed that all of the other globals set by _drupal_theme_initialize() are not reset currently.

Technically we'd have to back up and restore them similar to the existing originalTheme + originalThemeKey globals.

However, perhaps we should remove those backups/restores entirely, so that the theme is simply re-initialized from scratch (if the theme is needed after restoreEnvironment() to begin with)...?

sun’s picture

Attached patch implements #50 — i.e., global theme variables are unset both in prepareEnvironment() and restoreEnvironment().

damiankloip’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -1097,6 +1093,10 @@ private function prepareEnvironment() {
+    unset($GLOBALS['theme_info']);
+    unset($GLOBALS['base_theme_info']);
+    unset($GLOBALS['theme_engine']);
+    unset($GLOBALS['theme_path']);

+1 for this simple approach. I like it.

Small point: What do you think about just passing multiple variables in one call to unset instead? unset(one, two, three, ..)

The last submitted patch, 50: drupal8.test-theme-data.50.patch, failed testing.

sun’s picture

@damiankloip: I would have used multiple args to unset() normally, but in this case, every single of those unset() calls needs to be removed prior to 8.0.0, and therefore, other (possibly parallel) patches can remove individual ones more easily if they're on separate lines.

Glad to see that the simpler approach came back green, so let's go with that :)

jibran’s picture

How can one RTBC this patch?

sun’s picture

@jibran: Not sure how to interpret your question. Are you asking for tests? I don't think I want to add test coverage for this fix, because it's just a stop-gap fix... As mentioned in #54, none of these globals should exist to begin with, and we already have a range of theme system cleanup issues that are eliminating them one by one.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix

@sun thanks for the explanation. We are unsetting $_GLOBALS and it is green and it'll help a critical issue so RTBC.

damiankloip’s picture

RE #54, ok fair enough. Happy with that. +1 for this!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ad6f3ea and pushed to 8.x. Thanks!

  • Commit ad6f3ea on 8.x by alexpott:
    Issue #1770902 by sun, InternetDevels, mbrett5062, tim.plunkett: Theme...

Status: Fixed » Closed (fixed)

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