Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up from #1563620: All unit tests blow up with a fatal error.
<catch> berdir: how come we're not back in the parent environment by that point though?
<catch> I'd think $test->run(); should include messing with the db prefix both ways.
<berdir> catch: ah, one sec, this is actually something different and ok, yes.
<berdir> catch: well, it's easy to find out, I'll comment it out and see what happens ;)
<berdir> catch: I haven't touched that part myself yet
<berdir> catch: hah. It's simpletest's own verbose debug stuff that blows up, among other things
<berdir> catch: actually, that one is just because TableSortTest thinks it's necessary to do $this->verbose() on all kinds of stuff. should be cleaned up, but not there
<berdir> catch: yep, it's just that. for the current tests
<berdir> catch: we *could* remove it, fix TableSortTest and do something with verbose() in UnitTestBase
<berdir> catch: either forward to debug() or throw a not supported exception
<catch> berdir: that sounds much better tbh. This issue or a follow-up?
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff-1611430-37-43.txt | 1.14 KB | brad.bulger |
#43 | fix-verbose-1611430-D7-43.patch | 2.27 KB | brad.bulger |
#37 | fix-verbose-1611430-D7.patch | 2.26 KB | Stevel |
#34 | pre-set-verbose-url-1611430-34.patch | 2.06 KB | Berdir |
#31 | removal-of-l-1611430-30.patch | 882 bytes | Berdir |
Comments
Comment #1
catchSo we should be able to revert the following I think, if we can genuinely ensure that no module_*() functions are called by simpletest itself during unit test runs, and no theme() functions either.
Berdir also mentioned moving verbose() directly to WebTestBase which sounds great.
Comment #2
BerdirAlso:
To elaborate on simpletest_verbose(), that function is called twice, once from TestBase::run() to inject the current class name and once from TestBase::verbose() to actually generate a verbose log message. If we move that function into the test class, we can get the class name directly and don't need that weird injection stuff. Might be better suited for another issue, though, although I'm not sure if we can actually move it without merging.
Note that there is also a related bug report lying around because verbose() is currently broken on Windows due to \ in class names.
Comment #3
sunThis dependency actually is not new... it's just that some recent changes for D8 made it more obvious that the dependency exists (most probably drupal_container() combined with the theme system statics being swapped out with drupal_statics() by #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations).
Comment #4
BerdirWell, you can use debug() for "authoring and debugging tests", no?
IMHO, the way TableSortTest uses verbose() is wrong.
Comment #5
catchAssert functions - we should change those to use format_string(), same as custom assert messages in tests.
A different implementation for unit tests sounds reasonable.
Given the t() issue I agree this isn't a major bg, would be great if we could have unit tests completely explode if you make a not-really-unit test though.
Comment #6
Berdirverbose() related issues:
- #1587196: Simpletest can't handle namespaces when creating verbose output.
- #1588132: Simpletest verbose output url generation should be centralized.
Comment #7
BerdirOk, the attached patch merges simpletest_verbose() into TestBase and makes it UnitTest-safe by removing the l. l() is actually wrong there anyway, as we e.g. don't want language prefixes in there. We already have a full url and don't want to change it.
This duplicates both issues in #6 but one there is already RTBC and major so it might make sense to get that in first and then re-roll this.
Still think that TableSortTest shouldn't be calling verbose() like that but it's not necessary to remove it anymore.
I think we can safely say that unit tests must not use the theme system and t() works as long as locale.module isn't installed.
Comment #9
BerdirOuch. We can't call variable_get('simpletest_verbose', TRUE) during a test, obviously..
Comment #10
sunI think we rather want to put this into ::run(), since a mere instantiation of a test class does not necessarily mean that tests will be run (and thus, that a verbose directory will be needed).
Comment #11
BerdirI wasn't sure about that, I'm fine with moving it (back). The idea of having it in the constructor was that it's something that only needs to be done once. Actually even globally, so it might be possible to move it to a completely different place, like an initial run test preparation, both for the UI/Batch and run-tests.sh. Not sure...
Comment #12
sunThat central "test run initialization/preparation" is TestBase::run(). ;)
Comment #13
BerdirYes, test run. The verbose directory does not change based on the test class/run, so it could be done once instead of being repeated all the time.
Doesn't matter much, though.
Comment #14
sunThis patch looks ready to me after:
- moving the verbose directory setup code into ::run().
- Reverting the change/removal in UnitTestBase::setUp(). The theme registry needs to be re-injected, since any call to l() or t() in test assertions would throw a fatal error otherwise.
Comment #15
sunBetter title.
Comment #16
BerdirRe-roll of the requested changes.
IMHO, you can't call a function that relies on the theme registry in a unit test, that is against the concept of unit tests so I'd be ok with throwing a fatal error there (As mentioned above, t() does *not* trigger a theme/database lookup because no languages are enabled).
Comment #18
BerdirUps, re-added the $class = ... line.
Comment #19
sunAwesome, thanks @Berdir!
I also slept over the removal of theme registry injection in UnitTestBase::setUp() and of course we should remove that if possible. I merely wasn't sure whether verbose debug is the only blocker for removing it. So if you want to quickly add it back, we can remove it in this patch (sooorry). Or we can do that in a follow-up issue.
Comment #20
BerdirI'm fine with keeping it out of this patch, we can talk about further improvements in follow-up issues. I'm also going to mark #1588132: Simpletest verbose output url generation should be centralized. as a duplicate of this issue because this now also covers everything that one was about.
Comment #21
sun#18: simpletest-verbose-1611430-18.patch queued for re-testing.
Comment #22
catch#18: simpletest-verbose-1611430-18.patch queued for re-testing.
Comment #24
BerdirNot sure why the patch didn't apply anymore but git rebase was able to merge it just fine :)
Also re-added the removal of the registry injection since I had to do a re-roll anyway.
Comment #25
sunComment #26
sun#24: simpletest-verbose-1611430-24.patch queued for re-testing.
Comment #27
sunThis patch is required for #1705748: Convert simpletest settings to configuration system, since the current procedural simpletest_verbose() function is completely out of scope of the test that is actually executed, so it is not able to access the runtime configuration of simpletest (of the parent site).
Also, I think this should be backported. (we might have to keep the simpletest_verbose() in the backport though)
Comment #28
catchThanks! Committed/pushed to 8.x, moving to 7.x for backport.
Comment #29
BerdirNot sure what exactly you want to backport, simpletest_verbose() removal is 90% of this patch. Or do you just want to keep it as a empty wrapper or something like that?
Edit: Ah, do you mean removing the l()?
Comment #30
BerdirSo basically just this, right?
The theme_registry stuff that is removed here hasn't been backported yet.
Comment #31
BerdirSo basically just this, right?
The theme_registry stuff that is removed here hasn't been backported yet.
Comment #32
sunHm. I actually thought of backporting the whole thing - minus the removal of simpletest_verbose().
Essentially, moving/"duplicating" its functionality onto the class, as in D8, but keeping simpletest_verbose() for whatever wonky raw test/script is calling directly into it. (Everything should be using ->verbose(), but you never know...)
Comment #33
sunComing from #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config:
The file_create_url() causes module hooks to be invoked in upgrade tests from ->verbose() calls in the test, before update.php migrated the system data to the current core.
It's not clear to me why we need file_create_url() here -- how can we avoid it?
If necessary, we need to prepare and add a new $verboseDirectoryUrl property that contains the already resolved file URL up to the directory, so we only append the filename.
Comment #34
BerdirThis seems to work with manual testing, I think we don't have any verbose output tests.
Comment #35
sunThanks! Looks good to me!
Comment #36
webchickI don't think we can actually add tests for this, sadly, so committed and pushed to 8.x. Thanks!
Back to 7.x for backport.
Comment #37
Stevel CreditAttribution: Stevel commentedAn updated patch for Drupal 7, incorporating #31 and #34.
Comment #38
Stevel CreditAttribution: Stevel commentedRemoving tag because the backported patch is available.
Comment #43
brad.bulger CreditAttribution: brad.bulger commentedReroll of 7.x backport
This fixed a PDOException error I was getting from a verbose message in AggregatorUpdatePathTestCase.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!
In theory I think this should use check_plain() on the $url (since it's HTML output) but in practice I suppose it doesn't matter in this case.