Names of generated tables for tests exceeded 64 chars as reported at #13 #710858: Meta issue: fix the remaining PostgreSQL bugs

SQLSTATE[42P07]: Duplicate table: 7 ERROR: relation "simpletest322535field_data_field_s322535u2crryva_field_s322535u" already exists

So this patch changes field_name to field_file for tests

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Title: Fieldname in tests are too long for postgresql » Fieldname in tests are too long

Mysql just silently truncates so affected too.

yched’s picture

Agreed. Can we use a less common name than 'field_file', though ?

Also, I'm willing to bet there are other field tests where field names use $this->randomName()...

andypost’s picture

@yched maybe replace with "test_file" ? I walk through other field-related tests and found no consistency in names: field_1 field_2 field_type decimal52 does_not_exists... drupal_strtolower($this->randomName() . '_field_name')

Suppose that strtolower($this->randomName()) is enough for any test!

Should we replace all occurrences?

c960657’s picture

When #535440: Random strings in SimpleTest should not contain $db_prefix is fixed, the strings returned by $this->randomName() will be about 15 characters shorter.

andypost’s picture

@c960657 I talk not only table names but index names also.

Suppose all test fields should start with test_ prefix and should not use field_ which reserved for user's namespace

andypost’s picture

New patch fixes
- modules/file/tests/file.test
- modules/image/image.test
- modules/field/modules/field_sql_storage/field_sql_storage.test

Also extends test for indexes for pgsql and sqlite in field_sql_storage.test because now we have API to check indexes

This is very helpful in testing #720620: indexExists() for pgsql and sqlite does not prefix tablename and #718016: DatabaseSchema_pgsql::renameTable() needs to rename the indexes

andypost’s picture

Patch

andypost’s picture

And another one from list.test, I think all field modules covered

I see no reason to use drupal_strtolower() because strings are ascii

andypost’s picture

This depends on #722912: db_index_exists() must conform schemaAPI indexExists()

because all implementations of indexExists are different from db_index_exists and abstract schema class

Dries’s picture

1. If it depends on #722912: db_index_exists() must conform schemaAPI indexExists(), how come the tests in #8 pass? Would be great if you could provide more context on the dependency.

2. strtolower($this->randomName()) seems to be a very common pattern. Any reason why randomName() wouldn't always return lower-case strings? We strtolower() the randomName roughly 50% of the time ... could make for a really sweet clean-up IMO.

$ grep -r strtolower * | grep randomName | wc -l
     46

$ grep -r strtolower * | grep -v randomName | wc -l
      56

3. Technically, it would be better to write drupal_strtolower($this->randomName()) instead of strtolower($this->randomName()).

andypost’s picture

@Dries It depends and pass tests because using Database::getConnection()->schema()->indexExists() in the changed test so I think to revert this to db_index_exist() and glad to remove following comment in code once commited #722912: db_index_exists() must conform schemaAPI indexExists().

-    // We do not have a db-agnostic inspection system in core yet, so
-    // for now we can only test this on mysql.

Now about randomName() (used more then 200 times) it's most widely used then randomString() (6 times in all core test). Using of druppal_strlower() against strlower() is useless and much resource expensive because strings are not multi-byte as nature of randomName().

...The farther into the forest, the more wood

After digging into I found Cause - a bad documentation in #295864: SimpleTest: Change randomName() method to randomString()
It was planed that randomName() should be used for machine-readable-places and randomString() for user-input-data but most of test-writers are missed this point so now we need [drupal_]strlower() on result of randomName() also waiting for #535440: Random strings in SimpleTest should not contain $db_prefix

andypost’s picture

Reroll after commit #535440: Random strings in SimpleTest should not contain $db_prefix

Added small changes
- using randomString() for titles
- randomName(8) changed to randomName() because 8 is really default value now

EDIT: now it passes local tests on pgsql db with 2-character prefix 'd_'

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks andy.

Status: Fixed » Closed (fixed)

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