in modules/profile/profile.test

    $fid = db_result(db_query('SELECT fid FROM {profile_fields} WHERE title = "%s"', $title));

results in failed tests with errors like:

Unexpected PHP error [Unknown column 'single_simpletest_MODMgjmufz' in 'where clause' query: SELECT fid FROM simpletest488007profile_fields WHERE title = "single_simpletest_MODMgjmufz"] severity [E_USER_WARNING] in [/includes/database.mysqli.inc line 130]

How in the world did this not get noticed before?! These tests don't work at all! Do other tests have this too?

PHP 5.2.4, MySQL 5.0.45, Darwin 10.4.11 (intel)

CommentFileSizeAuthor
#14 separate-profile-tests-252920-14.patch9.75 KBfloretan
#13 separate-profile-tests-252920-13.patch8.63 KBpwolanin
#9 profile.test.patch47.1 KBfloretan
#6 profile.test.patch45.08 KBfloretan
#1 profile-test-quotes-252920-1.patch20.91 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new profile-test-quotes-252920-1.patch20.91 KB

ok, plus the redundant code is really annoying me.

here's an attempt to refactor to avoid redundancy and fix the errors.

Tests seems to pass, but this error makes me think all is not well:

user warning: Table 'dp70.profile_fields' doesn't exist query: SELECT f.name, f.type, v.value FROM profile_fields f INNER JOIN profile_values v ON f.fid = v.fid WHERE uid = 1 in /Users/Shared/www/drupal-7/modules/profile/profile.module on line 230.
floretan’s picture

floretan commented
Status: Needs review » Active

profile.test is a rare example of very diverse programming methodologies which have almost disappeared nowadays, such as:

  • in-test manual creation of roles and users, showing evidence that this code was written before drupalCreateUser() existed
  • multiple replication of identical methods like _rolesApi()
  • Polish comments. Did you know that "wartosci" means "value"? (thanks Wikipedia)
  • commented code

There's probably more fun stuff to be discovered here. I started fixing some of the code, but I'm almost wondering if it wouldn't make more sense to rewrite this test from scratch.

To reply to the original issue, I think profile.test needs a serious clean-up before we tackle the issues with those queries.

edit: it seems like I cross posted with the previous comment.

floretan’s picture

floretan commented
Status: Active » Needs review
pwolanin’s picture

pwolanin commented

@flobruit - if you want to undertake a major rewrite (which is clearly needed) please do. However, advise then on whether you're going to wait for: http://drupal.org/node/73874 and/or whether the current patch is of value.

floretan’s picture

floretan commented
Assigned: Unassigned » floretan
Status: Needs review » Needs work

The test currently breaks when applying the patch from #73874: Normalize permissions table because users and roles are created manually in the tests by referencing the permissions table directly. Once that is fixed, the two issues should not have any direct dependencies.

floretan’s picture

floretan commented
Status: Needs work » Needs review
StatusFileSize
new profile.test.patch45.08 KB

Here's a patch for a totally refactored profile.test (200 lines instead of 1000 lines of code) that tests the same functionality as the original profile.test.

There are still some improvements that can be done, but this is at least a solution that lets us move forward with #73874: Normalize permissions table.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

Looks pretty good - remove the trailing ?>

This function seems like to be broken:


  function deleteProfileField($fid) {
    // Delete field.
    $this->drupalPost("admin/user/profile/delete/$fid", array(), t('Delete'));
    $this->drupalGet("admin/user/profile");
    $this->assertNoText($new_title, "Checking deleted field $title");
  }
}

Since neither $title or $new_title are set. However, it doesn't seem that the function is ever even called - so there seems to be no checking for field deletion.

boombatower’s picture

boombatower commented

Just to confirm this test needs a rewrite. This was attempted, but never completed in http://drupal.org/node/242934.

floretan’s picture

floretan commented
Status: Needs work » Needs review
StatusFileSize
new profile.test.patch47.1 KB

The remaining issues have been fixed:
- field deletion
- trailing ?>
- assertion messages not wrapped with t()
- added test for field weight

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks flubruit -- good job.

pwolanin’s picture

pwolanin commented
Status: Fixed » Active

Trying to run this from the browser, it times out or dies before completing. When I run it via CLI:

php scripts/run-functional-tests.sh ProfileTestCase

I get:

FAILURES!!!
Test cases run: 1/1, Passes: 170, Failures: 276, Exceptions: 0

pwolanin’s picture

pwolanin commented

see also: http://drupal.org/node/254166

pwolanin’s picture

pwolanin commented
Priority: Critical » Normal
Status: Active » Needs review
StatusFileSize
new separate-profile-tests-252920-13.patch8.63 KB

This patch mostly just moves code around so that we have a common class with extra methods but no tests, and then a bunch classes with one test each.

At the least I can run these one-by-one without a PHP timeout. I tested all of them, and they work from the browser.

floretan’s picture

floretan commented
StatusFileSize
new separate-profile-tests-252920-14.patch9.75 KB

Refactoring this test into multiple classes is definitely good.

The test has a lot of overhead due to the too frequent switch between admin_user and normal_user. Moving the calls to $this->drulaLogin() to the appropriate place reduces the number of assertions by 50%, and the execution time by about as much. If needed, access check test should probably be integrated with profile field visibility tests.

dries’s picture

dries commented

I get the same as pwoladin:

$ ./scripts/run-functional-tests.sh ProfileTestCase
Test cases run: 1/1, Passes: 170, Failures: 276, Exceptions: 0

While this patch improves browser based testing, it degrades CLI based testing. The problem with splitting up the tests in multiple smaller classes is that it becomes tedious to run them from the command-line.

User story: I make some changes to profile.module and I want to run all profile module's test cases. run-functional-tests.sh ProfileTestCase is guessable, but run-functional-tests.sh ProfileTestCase ProfileTestFields ProfileTestSelect ProfileTestDate ProfileTestWeights. Even if it is guessable, it is a PITA to type.

Maybe the solution is to update run-functional-tests.sh so we can do run-functional-tests.sh profile (i.e. run-functional-tests.sh $module_name)? But how would that work for the XML-RPC tests in includes/?

pwolanin’s picture

pwolanin commented

@Dries - given that the CLI tests are not working, I'd indeed rather see a way to just specify a module name. Having to specify the class names is a little odd anyhow.

And most modules (user, etc) already have several tests.

dries’s picture

dries commented
Status: Needs review » Fixed

You're right. I've added a note to http://drupal.org/node/254166 and committed this CVS HEAD.

pwolanin’s picture

pwolanin commented
Status: Fixed » Needs review

It looks like the tests report themselves as belonging to a group (not a module), so it should be possible to specify the group name

So, it should be feasible to run all the tests belonging to a certain group (which usually is a single module).

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

Back to fixed.

boombatower’s picture

boombatower commented
Component: simpletest.module » tests

Change component is relation to http://drupal.org/node/253744.

Anonymous’s picture

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

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