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.
To continue our discussion from before...
On alpha6, any dbtng-driven queries run during simpletests use the normal site prefix, rather than the simpletest prefix. I noticed this when I realized that my tests were failing when my test module was pretending like it couldn't get any data out, and real data from the site itself appeared in the verbose output. Yowza.
Comment | File | Size | Author |
---|---|---|---|
#23 | use-static-class-variable-instead-of-global-at-parse.patch | 382 bytes | marvil07 |
#19 | dbtng-simpletest-prefix-914392-19.patch | 1.13 KB | neclimdul |
#18 | 0001-start-with-a-simple-test-v3.patch | 4.74 KB | marvil07 |
#17 | 0001-start-with-a-simple-test-v2.patch | 4.97 KB | marvil07 |
#15 | table_d7.test | 1.89 KB | mikeryan |
Comments
Comment #1
marvil07 CreditAttribution: marvil07 commentedThe related issue at vcs api queue is #893010: Fix tests (there is a little debug patch that let shows that prefix is not the right one)
Comment #2
sdboyer CreditAttribution: sdboyer commentedComment #3
mikey_p CreditAttribution: mikey_p commentedThis is probably due to prefixes in D7 being per connection instead of global, which is why things worked previously. I'll try to roll a patch for this tonight.
Comment #4
mikey_p CreditAttribution: mikey_p commentedBlargh. retagging
Comment #5
mikey_p CreditAttribution: mikey_p commentedI don't have any tests that use dbtng (yet), so I'll need someone else to try this.
Comment #6
neclimdulapplied patch and ran the VersioncontrolAccountStatusTestCase test. Everything passed!
Did get a small exception at the end though:
Looks like maybe we're trying to reset to the default database connection at the end of the test but it isn't working correctly.
Comment #7
eliza411 CreditAttribution: eliza411 commentedI verified with neclimdul that the blocker this issue represents is resolved. I'm going to mark this fixed and move the small exception to its own issue and encourage that testing open tickets for new issues as they arise.
Comment #8
eliza411 CreditAttribution: eliza411 commentedNew issue at http://drupal.org/node/955996
Comment #9
mikey_p CreditAttribution: mikey_p commentedSetting back to RTBC so that I don't forget to commit this. It may be a few days (but should definitely happen before the end of sprint 3) as I'd like to wrap this up in a beta2 release.
Comment #10
eliza411 CreditAttribution: eliza411 commentedGreat, thanks. I am getting the hang of process and what status means to module maintainers.
Comment #11
marvil07 CreditAttribution: marvil07 commentedI confirmed that neclimdul could run the versioncontrol tests, but I was not able to reproduce it, I mean make tests run.
He also told me he is using one path on top of its simpletest module: #890440: Backport latest SimpleTest code from D7, which is a patch for the simpletest core patch, so to test it, it's needed to re-apply it. But at the end I could not run successfully the tests.
So, here I am attaching a really tiny test module for dbtng based on the database_test module at D7 test suite. It only try to test a select with dbtng, that is what is used at the end on VersioncontrolEntityController::load(), so if that test pass, the VersioncontrolRepositoryUnitTestCase should be passing.
I am on latest cvs versions of all(core, autoload, dbtng, versioncontrol), so if someone can confirm the tests are running, the problem should be something in my sandbox, please help me to confirm it.
Comment #12
marvil07 CreditAttribution: marvil07 commentedPlease note that the last patch with the simple dbtng select on a test depends on the patch at #957262: Make sure autoload registry is setup, but after it I still can not see dbtng is working on simpletest.
Changing status, because I can not reproduce test working(hopefully the right thing to do).
Comment #13
mikeryanI'm struggling with getting Migrate tests working again with the latest dbtng/autoload, and even with the patch in this issue I'm getting nowhere. It appears to me that dbtng_table_exists() and $connection->schema()->createTable() do not respect the simpletest prefix - they seem to reference the non-prefixed db. The same code works in D7, and this works in D6 when run from a drush command. I can provide more details tomorrow, fading fast...
Thanks.
Comment #14
mikey_p CreditAttribution: mikey_p commentedMike I think the issue you are running into may be separate from some of the other issues that we are experiencing. Are you getting this when running a simpletest, or just in testing with a prefixed DB?
Comment #15
mikeryanI don't have a prefixed DB. When running migration commands in drush, table creation and dbtng_table_exists() work fine. In simpletest, createTable() creates table unprefixed, and dbtng_table_exists() matches the unprefixed tables. To demonstrate this, I'm attaching equivalent D7 and D6 tests of createTable() and db[tng]_table_exists(). At least for me, the D7 tests pass, but the D6 tests show the dummy table created without a prefix.
This is with the patch in comment #5. But, since hook_init() isn't called in simpletests, I guess that doesn't matter, and this is just an instance of the basic problem?
Comment #16
neclimdulTest passes for me, what is the error?
Comment #17
marvil07 CreditAttribution: marvil07 commentedAfter coming to a conclusion on #955996: Exception: The specified database connection is not defined: default I have updated this patch to include a base class which deal with two problems:
- at
setUp()
callhook_init()
manually- at teardown() remove the default connection to avoid having problems with the prefixes(see the comment at the patch)
I am moving back to "needs review", but note that I am now sure that patch at #5 is RTBC and it's actually a requirement for the patch I am attaching here.
Please note that after making versioncontrol tests inherit from
DBTNGTestCase
they also work for me finally \o/PS: I don't really reviewed patches at #15 :-(, so if tests are relevant please include them(but maybe it is a good idea to add the whole testing stuff from D7 at once).
Comment #18
marvil07 CreditAttribution: marvil07 commentedOopps, a minor comment removing :-p
Comment #19
neclimdulSeconding RTBC.
Updating #5 though and replacing the @ with an empty check.
So this patch + #18 = RTBC as far as I'm concerned.
Note: I still needed #957262: Make sure autoload registry is setup to get the test in #18 to pass.
Comment #20
mikey_p CreditAttribution: mikey_p commentedtagging for sprint 4
Comment #21
mikey_p CreditAttribution: mikey_p commentedI just added all the tests from D7. This should greatly help in testing things and making sure that everything works as it should. There are still a number of failures, but most of the tests seem to be passing okay. Currently the only thing required right now is the tests must require autoload and dbtng modules, in the order. For some reason the dependency system doesn't seem to work correctly, or work if the modules are out of order.
All of this code is in the DRUPAL-6--1 branch, and I plan to tag a beta2, or possibly beta3 as soon as it's had some folks using it and can confirm that it works.
Comment #22
mikey_p CreditAttribution: mikey_p commentedComment #23
marvil07 CreditAttribution: marvil07 commentedA minor follow-up about looking at the static class data member
self::$databaseInfo
instead of the global$databases
variable.Comment #24
marvil07 CreditAttribution: marvil07 commentedmikey_p just committed this on http://drupal.org/cvs?commit=448674
moving back to fixed
Comment #25
mikey_p CreditAttribution: mikey_p commented@mikeryan: FWIW the only way that seems possible to get this to work is to call Database::removeConnection('default') in the tear down method for your tests. While I haven't run into this issue, others have, and this seems to be what simpletest does in D7 to handle prefixing differently for each test case when the prefix info is stored with the DB connection.
Comment #26
mikeryanAll's well with the latest update from CVS, thanks!