Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem
- $GLOBALS['theme_key'] and $GLOBALS['theme'] from the parent site (test runner) are leaking into all tests.
Comment | File | Size | Author |
---|---|---|---|
#51 | drupal8.test-theme-data.51.patch | 3.35 KB | sun |
#50 | drupal8.test-theme-data.50.patch | 1.82 KB | sun |
#49 | drupal8.test-theme-data.49.patch | 1.17 KB | sun |
#45 | drupal-test_theme-1770902-45.patch | 1.6 KB | InternetDevels |
Comments
Comment #2
sunI 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.
Comment #3
tim.plunkettRemoving tests isn't the best solution.
Perhaps #1388546: Adding CSS or JS files twice changes weight would help?
Comment #4
sunWell, 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)
Comment #5
sunBut then again, I have better things to do than to fight over completely needless test code assertions ;)
Comment #6
tim.plunkettContrary 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.
Comment #7
sunHuh. 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.
Comment #8
sunAlrighty: 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:
drupal_get_css()
.drupal_get_css()
calls intodrupal_render()
, which calls intotheme()
, which "lazy-initializes" the theme if it hasn't been initialized yet.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.
Comment #9
sunPassed 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
Comment #10
tim.plunkettThat's a great fix.
Comment #11
webchickSeems consistent with what we do elsewhere for this kind of situation.
Committed and pushed to 8.x. Thanks!
Moving to 7.x for backport.
Comment #12
sunThanks!
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).
Comment #13
sunComment #14
sunAhem, *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 ;)
Comment #15
sunhah, seriously did not expect that patch to pass on the very first shot, great. ;)Ehhh, scratch that -- the comment was meant for another issue ;)
Comment #16
sunThis is just a very trivial follow-up to the original patch, so I hope you don't mind if I RTBC my own patch.
Comment #17
sunPing?
Comment #18
sun#14: drupal8.test-theme.14.patch queued for re-testing.
Comment #20
tim.plunkett#14: drupal8.test-theme.14.patch queued for re-testing.
EDIT: #1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting()
Comment #21
sun#14: drupal8.test-theme.14.patch queued for re-testing.
Comment #23
sunRe-rolled.
Comment #25
sunGeez. 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.
Comment #26
sunRe-rolled.
Comment #28
sunOops, 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.
Comment #29
sun#25: drupal8.test-theme.25.patch queued for re-testing.
Comment #30
sun#26: drupal8.test-theme.26.patch queued for re-testing.
Comment #32
markhalliwell#25: drupal8.test-theme.25.patch queued for re-testing.
Comment #33
mbrett5062 CreditAttribution: mbrett5062 commented#26: drupal8.test-theme.26.patch queued for re-testing.
Comment #35
mbrett5062 CreditAttribution: mbrett5062 commentedOK that last test run dramatically improved test results, but nothing changed in patch. Testing manually to see if I can figure this out.
Comment #36
mbrett5062 CreditAttribution: mbrett5062 commented@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.
Comment #37
mbrett5062 CreditAttribution: mbrett5062 commentedOK 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.
Comment #38
mbrett5062 CreditAttribution: mbrett5062 commentedhhmmmm, and they pass here. That's weird, but cool.
Comment #39
chx CreditAttribution: chx commentedSo how do we test this...?
Comment #40
chx CreditAttribution: chx commentedAlso, I am not sure whether it's correct to run theme init -- maybe? Why not just unset the globals?
Comment #41
mbrett5062 CreditAttribution: mbrett5062 commentedSorry @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?'.
Comment #42
Berdir#36: drupal-test_theme-1770902-36.patch queued for re-testing.
Comment #44
penyaskitoComment #45
InternetDevels CreditAttribution: InternetDevels commentedReroll
Comment #46
sunI 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.
Comment #48
sunComment #49
sunRe-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.
Comment #50
sunFurther 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)...?Comment #51
sunAttached patch implements #50 — i.e., global theme variables are unset both in
prepareEnvironment()
andrestoreEnvironment()
.Comment #52
damiankloip CreditAttribution: damiankloip commented+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, ..)
Comment #54
sun@damiankloip: I would have used multiple args to
unset()
normally, but in this case, every single of thoseunset()
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 :)
Comment #55
jibranHow can one RTBC this patch?
Comment #56
sun@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.
Comment #57
jibran@sun thanks for the explanation. We are unsetting $_GLOBALS and it is green and it'll help a critical issue so RTBC.
Comment #58
damiankloip CreditAttribution: damiankloip commentedRE #54, ok fair enough. Happy with that. +1 for this!
Comment #59
alexpottCommitted ad6f3ea and pushed to 8.x. Thanks!