This is a child issue of #3057420: [meta] How to deprecate Simpletest with minimal disruption
Problem/Motivation
We want to decouple the test running infrastructure from the simpletest module itself.
Many tests use Drupal\Core\StreamWrapper\PublicStream\TestFileCreationTrait::getTestFiles()
to generate temporary fixture files.
TestFileCreationTrait::getTestFiles()
uses some files within the simpletest module as fixtures.
We can easily see this in #3075490-11: Move simpletest module to contrib where we experimentally remove the simpletest module and then run a druaplci test build. Most of the test fails are related to TestFileCreationTrait
.
Proposed resolution
Move the fixture files to a new location.
Change Drupal\Core\StreamWrapper\PublicStream\TestFileCreationTrait::getTestFiles()
to use the new location.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.txt | 2.74 KB | Mile23 |
#14 | 3080163_14.patch | 27.12 KB | Mile23 |
Comments
Comment #2
Mile23Comment #3
Mile23Comment #4
volegerLet's check the option when we move the files into
/core/tests/fixtures/files
folder.Comment #6
volegerLet`s fix failed tests
Comment #8
Mile23Fixes some tests.
I couldn't figure out the migration ones.
Comment #9
Mile23Not sure where that previous interdiff came from... Here's the real one.
Comment #11
LendudeString length changed, so the s:29 is wrong, hence the unserialize failure in the migrations I assume
Comment #12
LendudeLets see how this does.
Comment #14
Mile23The one remaining fail is
Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6Test
.It turns out that whoever made that test base was one step ahead of us, and duplicated the fixture files into their own directory, mimicking the path to the simpletest fixture directory. See
Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase::getSourceBasePath()
. Congratulations to contributors to #2867304: Convert web tests to browser tests for migrate_drupal_ui module :-)So this patch rearranges the fixtures in
core/modules/migrate_drupal_ui/tests/src/Functional/d6/files
to mimiccore/tests/fixtures/files
like the rest of core.Comment #15
volegerGreat) changes looks good. +1 for rtbc
Comment #16
Mile23This is a child issue of a critical meta: #3057420: [meta] How to deprecate Simpletest with minimal disruption
Comment #17
larowlanSo I was thinking about these files in that folder, should we have a .htaccess file in there with a deny from all directive to prevent people using them to fingerprint the version of Drupal?
Comment #18
LendudeI think #17 is a little out of scope here? Isn't that something we should do/discuss in a broader scope? Not sure what the added value would be doing it just here (but maybe I'm missing something).
Comment #19
mondrakeRelative paths assume that cwd while running the test is Drupal root. That's fine for DrupalCI bots, but other testing workflow may fail in this context. IMHO we should have absolute paths and prepend an app.root or a __DIR__dance here (and all other cases).
Comment #20
Lendude#19 sounds like a good improvement, but also sounds out of scope here, the same problem already exists in HEAD. Can we do that in a follow-up? Or is there a good reason that this needs to be done here?
Tentatively setting back to RTBC.
Comment #21
mondrakeProbably this case is more evident: I suspect (but am not sure)
drupal_get_path('module', 'simpletest')
will return an absolute path whereas after the change it will be relative. Not changing from RTBC, but I think #19 would avoid any risk.Comment #22
mondrakeI tested patch in #14 by running PHPUnit from within the core subdirectory
and all turned OK, so my concerns in #19 and #21 are not relevant. All good. Sorry for the noise.
Comment #24
catchI thought about copying these instead of renaming, but can't really imagine that contrib is relying on these fixtures for tests. If it turns out they are, we might need to copy them back again, but committing this as is and making a note here.
Comment #25
Mile23Thanks, @catch. Do we need a CR here?
Comment #27
xjm