Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.This is a tough issue. We definitely want better test coverage. Unfortunately these 2 scripts are fairly untestable and broken and the tests are essentially broken and tightly coupled to Simpletests mocked Drupal bootstrap. This fact is blocking #2016629: Refactor bootstrap to better utilize the kernel because that issue closes the loophole that lets the current hack function.
Backstory:
This probably goes back further but in recent history, trior to #2248985: ScriptTest fails on Windows, runs against parent site, this test passed through a fluke because testbots and webtests have a default site install. Also both scripts only work with the default site, and don't support multisite setups. That issue fixed is so we didn't need the base site installed but we still can't run these scripts on multisites.
The problem currently is that this test does a terrible thing and sidesteps out cli test and includes the scripts directly, cleverly relying an a quirk of the interaction of DUTB and the current bootstrap. DUTB fakes a booted environment populated with some basic Settings so tests can run. This script relies on that and the fact that if you try to re-bootstrap currently it does nothing.
In #2016629: Refactor bootstrap to better utilize the kernel however, booting a kernel loads the settings for the request being handled directly into Settings's storage, which because of the way the Settings class works erases all previous settings. In side a DUTB test, this means there are no longer any settings at all because there is no settings.php file and rebuild_token_calculator.sh fails to fetch a hash_salt causing an exception.
For now, this issue removes the test to unblock the kernel bootstrap issue. We'll need to follow up with fixes to the scripts and with that provide a way to actually test these scripts as well as some better tests of the code being executed.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | system.scripttest.15.patch | 3.19 KB | sun |
| #8 | system.scripttest.8.patch | 3.07 KB | sun |
| #6 | system.scripttest.6.patch | 1.87 KB | sun |
| #4 | system.scripttest.4.patch | 1.87 KB | sun |
| remove_SystemScriptTest.patch | 4.41 KB | neclimdul | |
Comments
Comment #1
neclimdulComment #2
sunremove_SystemScriptTest.patch queued for re-testing.
Comment #4
sunRedone from scratch against HEAD.
Comment #6
sunComment #8
sunMeh, OK, let's include the garbage to satisfy testbots.
Comment #9
sunComment #11
sunThe issue summary is still accurate and applies in even worse ways to #2304461: KernelTestBaseTNG™ - because there is no "parent site" that the test could silently fall back to. Since the silent fallback no longer exists, this test and the workaround in the scripts is no longer able to work at all.
In case it helps, the proper way for implementing (and testing) CLI scripts/commands is #2242947: Integrate Symfony Console component to natively support command line operations
Therefore, let's remove this test.
Comment #12
sunComment #13
neclimdulComment #15
sunRe-rolled against HEAD.
Comment #16
alexpottAs much as it pains me to remove these tests they are giving us a false sense of coverage. One issue is that these scripts are totally not multisite compatible which makes them very hard to test because we can't point them properly at the site under test.
So I'm going to commit this and set the issue to active and tag it "revisit before release" so we don;t forget to test these scripts and perhaps come up with a better way.
Committed 7f4876c and pushed to 8.0.x. Thanks!
Comment #18
sunMoved into separate issue: #2322877: Remove password-hash.sh and rebuild_token_calculator.sh
Comment #19
sun