Problem/Motivation
Currently, migrate_drupal unit tests which extend MigrateSqlSourceTestCase -- which is pretty much all of them -- rely on the fake DB driver in core. Unfortunately, this driver is a) extremely limited in what it can support, which limits what we can test, and b) doesn't always behave just like a real database would, which is extremely bad since that will affect the results of the tests and can lead to unreliable results. To me, these are critical problems that render the fake driver unsuitable for unit testing.
Proposed Resolution
SQLite to the rescue! It is possible for SQLite to create an in-memory database which ceases to exist when the connection is closed. In-memory databases are totally compatible with PDO and Drupal's database layer. They obviously require the PHP SQLite extension, but that's acceptable for testing purposes. In fact, parts of Drupal Module Upgrader's test suite use this exact approach. I propose that migrate_drupal's unit tests ditch the fake database driver in favor of the core SQLite driver and an in-memory database. This will greatly increase the robustness and accuracy of the unit tests.
Remaining Tasks
Review and commit the patch.
API Changes
None.
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-2499835-33-36.txt | 1010 bytes | phenaproxima |
#36 | 2499835-36.patch | 122.92 KB | phenaproxima |
#33 | 2499835-33.patch | 123.33 KB | phenaproxima |
#31 | interdiff-2499835-23-30.txt | 2.49 KB | phenaproxima |
#31 | 2499835-30.patch | 124.1 KB | phenaproxima |
Comments
Comment #1
phenaproximaComment #3
chx CreditAttribution: chx commentedGreat work! I recommend changing the comment in UserTest to
getDatabase() will not create empty tables, so we populate with some data even if irrelevant
or somesuch, the current language does not mesh with the intended level of professionalism of core.For the sake of history, Fake was written in 2013 predating the sqlite option #1808220: Remove run-tests.sh dependency on existing/installed parent site so we couldn't rely on sqlite because the bots didn't have it yet. Probably it should now be removed from core as broken. Oh well, at least we tried.
Comment #4
chx CreditAttribution: chx commentedAlso, reset($rows) is twice in getDatabase.
Perhaps not in this issue but this could and should be extended to replace Fake with this in-memory sqlite version where the relevant insert/merge classes create their tables (while sqlite requires a schema it's typeless; the column specification is merely a type affinity but the values themselves can provide another affinity) and query return peacefully if a table is not found.
Comment #5
phenaproximaRefactored MigrateSqlIdMapTest to work with SQLite.
Comment #6
chx CreditAttribution: chx commentedneeds doxygen.
Do we need those arguments now? Seems not.
This repeats. Perhaps a function in the base then?
MMmmmm I guess. Perhaps serialize([]) ? You even changed a serialize(array()) to this, why...? Yes, I see it's in HEAD too but ... let's standardize on serialize([[]) please.
Perhaps the "return value" needs to be countable? I think that's what you meant.
Core uses the word 'provider' in the name of the provider method in 410 out of the 435 provider methods. I'd say we should too.
Comment #7
cilefen CreditAttribution: cilefen commented@phenaproxima I suggest using the new array [] syntax if you are reformatting or adding arrays.
Comment #8
phenaproximaAll fixed!
Comment #9
cilefen CreditAttribution: cilefen commentedThere are a few more array()s here and in some later files in the patch.
Comment #10
phenaproximaSwitched to short array syntax in all the tests I modified.
Comment #11
chx CreditAttribution: chx commentedI think this is good to go then.
Comment #12
alexpottSo no we have no usages of Fake in Core? I think this patch should also remove it.
Migrate tests that dependon SQLite should fail with a requirements error if it is not installed. See TestBase::checkRequirements()
Comment #13
phenaproxima@alexpott, none of the tests depend on SQLite specifically. What they depend on is having a proper database to play with. The reason this patch switches from Fake to SQLIte is because Fake itself is broken, and SQLite is a convenient, functional replacement with the crucial advantages of Fake (operating only in memory) and none of the problems. Setting an SQLite requirement on every test doesn't really make a lot of sense to me, since virtually all of them depend on the presence of a database, and they could be run against MySQL or another similar anyway (except that's not feasible for unit testing).
However, after grepping core for "FakeConnection" and finding no additional mentions, I'm absolutely delighted to remove it. :) Fix attached.
Comment #14
phenaproximaRenaming the issue.
Comment #16
phenaproximaLet's greenify that.
Comment #17
chx CreditAttribution: chx commentedLet's do this, then.
Comment #18
alexpottThis creates a dependency on sqlite - if environment does not have sqlite this will fail hard. We should add a set up method to check that the PHP
sqlite3
extension is installed. And previous these tests had no dependency on any database.There is a ton of changing array syntax from
array()
to[]
in the patch which is a shame since its large enough as it is.Comment #19
phenaproximaNot true. They depend on a database, which the Fake driver was emulating. To be sure, they were not depending on any specific database, and they still don't. (That's the whole purpose of Drupal's database layer, right? :) getDatabase() makes the executive decision to supply them with an in-memory SQLite database. So adding a check for the SQLite extension is reasonable, but it shouldn't go in the setUp() method since that will affect every test, including the ones which don't need a database.
@cliefen requested this in #7 and #9.
Comment #20
phenaproximaAdded a check for the
pdo_sqlite
extension to the getDatabase() method.Comment #21
dawehnerIts odd, I mean we are testing something related to a database. Aren't we doing that in million other tests as well, so yeah I agree with @alexpott that we should not need something special, but I see you are using a pure unit test here.
Comment #22
phenaproximaWell, they're technically integration tests, since they're now interacting with a non-mocked database. Once KTBNG lands, we could change them to use prefixed tables in MySQL or Postgres instead, if we want to. The most important thing here, IMO, is to stop using the Fake driver and kick it out of core before anyone else decides to use it, because it is both redundant and broken.
Comment #23
chx CreditAttribution: chx commentedThese two points are definitely just my opinion but these are the reasons these tests are as they are.
It is important to see the migrate Drupal 6 sources being completely independent of Drupal 8. This makes KernelTestBase less usable. There's too much of D8 there. That alter in the select is quite wrong because it introduces a potencial circular dependency: load module -- try to load from cache -- select tries to alter -- modules are not loaded... kaboom! it doesn't because the cache doesn't add alter tags ATM. Because of this I have reverted the mock module handler from the patch and made Select Drupal independent again. DBTNG being Drupal independent was an important design consideration...
On the other hand I have serious problems with many unit tests. See, a typical query method does $this->select('files', 'f')->fields(... and that's it. Mostly. If you write $select_mock->expects()->method('fields)->willReturnSelf() or somesuch you could just as well parse the file and whether the method equals to a pre-stored copy of it character by character, it would roughly be equivalent. You are copying the code with a slightly altered syntax. Utterly useless. Whether the query actually selects anything useful you can only guess since the test is written as a copy-paste of the method tested. I'd rather create a database, put data in it, and see what I retrieve makes sense. If you want to be fancy: I do not see the point in unit testing a function with a cyclomatic complexity of 1. I know this is a personal thing but ... this is what you got when you accepted me as the migrate architect. You could, of course, rewrite this as "pure" unit tests. It's not something I would do but since I am not coding this, feel free. Same for using KTB.
Comment #26
chx CreditAttribution: chx commentedWeird bot fail.
Comment #27
dawehnerI totally agree that these tests don't make sense if they mock the database connection ...
Comment #28
benjy CreditAttribution: benjy at CodeDrop commentedI agree with this, pointless noise in the patch which in turn made it much harder to review.
What changed around alterTags in that we now need an empty check instead?
Why do we check for the Drupal class, is it the container we should be testing for?
Isn't this what crreateSchemaFromRow() does?
This could do with a comment, it just casts the values to a string?
So the tests only work with the pdo_sqlite extension?
Comment #29
phenaproximaI apologize. It was requested, and encouraged, and I did it. I'll be more careful in the future.
@chx can answer these better than I can, since he made those changes.
Yes, except the one you quoted only creates fields that don't already exist in the already-existing table. I could probably streamline this, though.
It's to get around a weird thing that PDO does. @chx and I looked into it the other day. I agree that a comment is very much in order.
No -- only getDatabase() requires the pdo_sqlite extension. Which makes sense to me, since if a test is calling getDatabase(), it needs...a database. If a test never calls getDatabase(), it will never hit that requirement.
Comment #30
phenaproximaAdded a comment about the string casts.
Regarding #28.1, at the moment there's not much difference between isset() and empty(), because $this->alterTags is undefined until someone adds a tag. Since empty() does an implicit isset() check, this is just a bit of future proofing for when alterTags is actually predefined on the Query base class.
And regarding #28.2,
I have a feeling (but I'm not certain) thatIt's because DBTNG may eventually be split out as a component. (Confirmed by @chx.)Comment #31
phenaproximaUgh. Patch this time.
Comment #32
chx CreditAttribution: chx commentedTo quote #23
I am not sure what else needs to be said? Hardwiring Drupal:: there is probably a bug and just calling moduleHandler without try-catch, as explained, again, in #23 is definitely a bug.
Comment #33
phenaproximaAs per discussion on IRC, reverted @chx's changes in #23. They should be addressed when DBTNG is split out into a component, but it's out of scope for this issue.
EDIT: I didn't include an interdiff because the changes are the exact inverse of #23. In fact, I used
git apply --reverse interdiff-23.txt
to generate this patch. :)Comment #34
benjy CreditAttribution: benjy at CodeDrop commentedOK reviewed again, i'm glad we removed the stuff from DBTNG from this patch. I think this is ready and a good clean-up.
Comment #35
alexpottWell this is always going to be skipped now. Since the database is sqlite. What's the point of having this test here? Should we move it to a KernelTestBase test? Also what does this mean about migrating from Drupal installations that use SQLite?
Comment #36
phenaproximaAs per discussion with @alexpott on IRC, removed the testGetQualifiedMapTableNoPrefix() method. We'll restore it when this test case is converted to a kernel test, after #2304461: KernelTestBaseTNG™ lands. (Followup issue forthcoming.)
Comment #37
phenaproximaFollow-up issue made. #2503183: Convert database-driven tests to kernel tests
Comment #38
phenaproximaBack to RTBC, with @alexpott's blessing.
Comment #39
alexpott618 insertions, 2412 deletions.
nice work! Less to maintain for all of us. Committed fa2a7a5 and pushed to 8.0.x. Thanks!Migrate is not under beta evaluation.
Comment #41
bzrudi71 CreditAttribution: bzrudi71 commentedNice cleanup! Sadly, this causes a new fail in MigrateSqlIdMapTest for all non MySQL databases (PG-bot / SQLite-bot). Do we just need a follow-up here or new issues for both DB's?
Drupal\Tests\migrate\Unit\MigrateSqlIdMapTest 28 passes 1 fails
Comment #42
alexpott@bzrudi71 let's handle this in a new issue.
Comment #43
alexpottCreated #2504417: Fix Drupal\Tests\migrate\Unit\MigrateSqlIdMapTest::testGetQualifiedMapTablePrefix()
Comment #45
fgmI would say this needs a change record, but I cannot set this status, for some reason : this class was there for a long time, and initializing this SQLite connection uses significantly more code than initializing the FakeConnection did, so there is a risk to have the initialization code be replicated in various guises.
I met this problem in the MongoDB driver, FWIW.
Comment #46
benjy CreditAttribution: benjy at CodeDrop commented@fgm, thanks for that, do you want to write a change record and post it here?