Feel free to change to a feature request or support request as needed.
My understanding was that all tests are run in a clean drupal environment but that is not the case. I just did a heavy debugging session to finally realize that a node_revisions test was failing because i had set a personal timezone in my account and thus a format_date() call was returning a different value for the simpletest user than for myself in the .test file which runs under whomever is logged in. It looks like setUp() makes a new drupal install and then runs the test but i can't figure out why my own uid is active in the .test file.
I hope I am seeing this right and not making a fool of myself.
Comment | File | Size | Author |
---|---|---|---|
#23 | 283246-start-from-a-clean-user.patch | 3.29 KB | boombatower |
#17 | 283246-start-from-a-clean-user.patch | 2.23 KB | Damien Tournoud |
#16 | 283246-DWTC-reset-user-D7.patch | 1.71 KB | Dave Reid |
#13 | simpletest_283246.patch | 1.69 KB | drewish |
#5 | 283246-cleanup-user-3.patch | 1.4 KB | Damien Tournoud |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedmoshe, can you try something like this?
Comment #2
chx CreditAttribution: chx commentedAnother issue where $GLOBALS/static (which are the very same mechanism in PHP just static has a scope limitation btw.) causes problems. drupalLogin needs to do a full user switching: disable session writes, switch users. Same for drupalLogout (if we have one but i suspect we do).
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commenteddamien - that looks good at first glance. could you add the session write code as mentioned at http://drupal.org/node/218104
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks for the pointer. Here's an updated patch.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedOn IRC, moshe suggested to simply load the uid 1 from the tested site. So moved to loading after variable cleaning. Here is an updated patch.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedThis fixed the revisions failures and created no new failures. Code looks good so RTBC.
Comment #7
chx CreditAttribution: chx commentedAgreed and then some.
Comment #8
Dries CreditAttribution: Dries commentedThe problem with this is that we might have to pull similar tricks for every global in every module that one wants to test (not just the core modules). This doesn't look like a scalable solution ... a more generic fix is in order, IMO. For example, does it make sense to iterate over every global and to unset it? :P
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe really should only run testing from a clean source environment. PHP doesn't allow unloading functions, so we can't really guarantee that except by controlling every aspect of that environment.
What I would do: add a test.php in Drupal root directory, bootstrap a controlled environment from that (like what's done at the top of install.php), and only then create the test environment.
Comment #10
Dries CreditAttribution: Dries commentedI think this needs a bit more thought and discussion. The proposed patch does not guarantee a clean environment at all.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedDamien's suggestion of a test.php is a good start. The details certainly need to be fleshed out. Fixing the 'clean environment' problem is a really big reworking of simpletest.
I didn't really intend to fully solve that when I started this issue. So, I am changing the title and setting to RTBC based on new title. The node revisions test has a bug and this fixes it, without creeping scope too much.
Comment #12
lilou CreditAttribution: lilou commentedDamien's patch #5 still applied to CVS HEAD.
Comment #13
drewish CreditAttribution: drewish commentedhere's a clean re-roll, still makes all the tests pass.
Comment #14
drewish CreditAttribution: drewish commentedwon't apply now that session_save_session() is drupal_save_session()
Comment #15
Dave ReidMarked #345979: Node revision tests fail with timezones as a duplicate of this bug, although progress has seemed to have stalled here.
Comment #16
Dave ReidRerolled for HEAD. If this isn't fixed somehow, I see it coming back to bite us once a lot of contrib modules get testing in.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedBetter reroll of the same idea.
Comment #18
Dave ReidThanks Damien, that's much better for documentation and obviously your diff isn't being wonky on you today. :) All tests are a green from me, let's see what testing bot says.
Comment #19
Dries CreditAttribution: Dries commentedI'm still not 100% on this because it only solves this problem for $user and not for the entire environment.
Comment #21
boombatower CreditAttribution: boombatower commentedThis is holding up test bot: #348111: Upload.test fails when run from run-tests.sh.
Comment #22
boombatower CreditAttribution: boombatower commentedUpdate session test to reflect changes.
This also correct upload.test failure.
Comment #23
boombatower CreditAttribution: boombatower commentedpatch..
Comment #24
Dave ReidConfirmed this fixes node revisions and upload tests both logged in when running simpletest, logged out running simpletest, and running tests from command line as well.
Comment #25
boombatower CreditAttribution: boombatower commentedAs a note the bot will be forced to sit idle until this patch (or comparable solution) makes it in.
I vote for getting this in and opening an issue to discuss a more scalable solution, rather than leave testing bot in a useless state.
Comment #26
catchI've been running into this bug as well recently, with random test failures people start mistrusting the test framework in general, and I'd rather have our patches tested and coverage increase with some not-perfect stuff in simpletest.module making that possible. Also I'm more interested in tidying up some of the (still, hideously) ugly core tests themselves than simpletest internals.
Comment #27
boombatower CreditAttribution: boombatower commentedlong term solution: #348455: Provide a scalable way to ensure a clean testing environment for SimpleTest
Comment #28
webchickThis is one of those "between a rock and a hard place" issues.
Dries is 110% bang-on that this is simply fixing a symptom of the overall larger problem of "tainting" the test environment. And we really do need to solve this in a clean and proper way.
Unfortunately, when #339929: Move node links into $node->content was committed, it introduced this little line:
And due to the lengthy sleuthing done by boombatower in #348111: Upload.test fails when run from run-tests.sh, we see that this is yet another symptom of this bug. And we can't turn back on the testing bot without it.
When testing bot is disabled, core committing essentially stops, because the alternative is running the entire test suite each patch which takes 20+ minutes, or closing your eyes and committing and hoping you didn't break an edge-case somewhere.
So... for now, I've committed this. :\ And I've asked boombatower to start #348455: Provide a scalable way to ensure a clean testing environment for SimpleTest which is where interested parties should go to fix this in the *proper* way so we don't need any more of these special-case things.
(Dries, please don't hit me. ;) Though if you feel strongly about it, I would be fine with a roll-back, but we probably need a more concrete plan of attack so we can unleash our coders at it.)