The tests for the tracker, profile and search modules all call drupalEnableModule() directly, rather than passing the name of modules to be enabled as a parameter to parent::setUp().

Since I'm responsible for introducing this non-standard code in two of these tests, it's also my job to provide a fix, so here it is.

CommentFileSizeAuthor
#11 remove_drupalModuleEnable.patch7.62 KBfloretan
#3 remove_drupalModuleEnable.patch6.43 KBfloretan
drupalModuleEnable_misuse.patch2.31 KBfloretan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs review » Reviewed & tested by the community

Patch looks good. All three tests pass before and after the patch.

The changes also make the tests more consistent and use the setUp method correctly.

Dries’s picture

Dries CreditAttribution: Dries commented

Should we document this 'common pattern' in the PHPdoc of drupalModuleEnable()?

I've made the same 'mistake' for some of my tests ... I didn't realize their was a pattern until I saw this issue.

floretan’s picture

floretan CreditAttribution: floretan commented
Title: Misuse of drupalEnableModule() in some tests » Remove drupalModuleEnable() from simpletest
Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs review
FileSize
remove_drupalModuleEnable.patch6.43 KB

After some more investigations, I found that we don't really need drupalModuleEnable(). Here are my reasons:

  1. There is currently no place in core where drupalModuleEnable() is called outside of a setUp() method (the only exception is in help.test, where a call to module_exists() is much cleaner).
  2. If we want to test the enabling/disabling of modules, it should be done by using drupalPost('admin/build/modules') directly.
  3. DrupalWebTestCase::setUp() doesn't even call drupalModuleEnable() to enable modules.
  4. drupalModuleDisable() could go as well, it is never called.
  5. The property DrupalWebTestCase::_modules is never used outside of drupalModuleEnable/Disable(). It could go too.

Here's a patch to remove drupalModuleEnable(), drupalModuleDisable() and the _module property. That should keep good developers like Dries from inadvertently using "non-standard" patterns :-)

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

That's funny, I made the exact same point in #260486: Weird 'phantom' errors after running tests, but with a slightly different rationale. I didn't even realized that DrupalWebTestCase::setUp() doesn't even call drupalModuleEnable() to enable modules.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs review » Reviewed & tested by the community

Been talking about this for sometime, not sure why it wasn't ever done.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Component: tests » simpletest.module
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I've removed drupalModuleEnable() from the documentation at http://drupal.org/node/265762 on the assumption this'll go in, and that if it doesn't for some reason, it's unlikely to remain a public API function.

Dries’s picture

Dries CreditAttribution: Dries commented

I applied this patch, and tried running all the SimpleTests twice. For some reason, they don't seem to complete anymore. The tests stop running, or so it seems. No SimpleTest summary was shown at the end so I suspect something crashed along the way. The last bit shown on the screen was (I'm using the command line script):

85) File is located in proper directory at [/Users/dries/Sites/drupal-cvs/modules/upload/upload.test line 310]
	in testWithGDinvalidDimension
	in Upload user picture
	in Upload

I un-applied the patch, and ran all SimpleTests twice again. Both times, the tests completed and a summary was shown.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

I'm getting this: Fatal error: Call to undefined method XMLRPCValidator1Test::drupalModuleEnable() in xmlrpc.test on line 17

Running everything except XMLRPC works fine so it looks like that one's just left over.

floretan’s picture

floretan CreditAttribution: floretan commented
Status: Needs work » Needs review
FileSize
remove_drupalModuleEnable.patch7.62 KB

My bad, I modified xmlrpc.test but accidentally forgot to include it in the patch.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Test all run now, so this seems fine.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs review

Great. Committed to CVS HEAD. Thanks flobruit.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs review » Fixed

I assume this is fixed?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.