Similar as case of #322457: [DBTNG + simpletest]: queryTemporary() is buggy and need simpletest test case, queryRange() is now missing simpletest test case (or maybe I miss that? please correct me if possible...).

It seems to be a good idea for an additional test case, because each database engine coming with different implementation, and simpletest can give a centralized testing for it :D

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Hm, right you are. Can you take a shot at writing a few tests? It's probably best to give that its own class, since the database test setup method is really slow. :-(

hswong3i’s picture

Assigned: Unassigned » hswong3i
Status: Active » Needs review
FileSize
2.56 KB
hswong3i’s picture

Title: [DBTNG]: queryRange() need simpletest test case » [DBTNG + simpletest]: queryRange() need update and simpletest test case
FileSize
4.36 KB

Patch update:

  1. Remove legacy backward compatible code.
  2. Update for better simpletest test cases.
catch’s picture

Status: Needs review » Needs work

hswong3i, in case you haven't noticed, we still have a few hundred queries in core which require this backwards compatibility layer and need converting to the new API.

hswong3i’s picture

Status: Needs work » Needs review

@catch: just take it easy that we are now able to remove these "duplicated" backward compatible code :-D

db_query_range() is now protected with backward compatible function _db_query_process_args():

function db_query_range($query, $args, $from = 0, $count = 0, $options = array()) {
  if (!is_array($args)) {
    $args = func_get_args();
    array_shift($args);
    $count = array_pop($args);
    $from = array_pop($args);
  }
  list($query, $args, $options) = _db_query_process_args($query, $args, $options);

  return Database::getActiveConnection($options['target'])->queryRange($query, $args, $from, $count, $options);
}

Therefore we don't need to run those "tiny and incomplete" backward compatible code within queryRange() again...

On the other hand, db_query_range() is mainly used by pager_query(). By checking CVS HEAD, we can find that dblog.admin.inc line 111 is now using legacy syntax:

  $result = pager_query("SELECT COUNT(wid) AS count, message, variables FROM {watchdog} WHERE type = '%s' GROUP BY message, variables " . tablesort_sql($header), 30, 0, "SELECT COUNT(DISTINCT(message)) FROM {watchdog} WHERE type = '%s'", $type);

When checking admin/reports/pages with apply of this patch, it should still running correctly :-)

Crell’s picture

Status: Needs review » Needs work

hswong is correct that the BC layer is handled elsewhere now. That's older BC code that has since been centralized, so it's not needed anymore. That doesn't relate to unit testing, though, so should probably be a separate patch. Also there's no need to break the query() method call into two lines; it's fine as one.

For the unit tests themselves, on visual inspection:

- There's no need to iterate the result. Instead, do $records = db_query(...)->fetchAll(), to get back the full result set as an array. You can then just do count($records) to get the number of records returned.

- Don't use assertTrue() here. It's cleaner/more semantic to use assertEqual(count($records), 2, t('message here'));

- As a convention I don't think we're going to support unnamed placeholders; in fact the documentation doesn't even mention that they exist. :-) Go ahead and drop that test, as it's unnecessary.

Thanks, Ed!

hswong3i’s picture

Status: Needs work » Needs review
FileSize
4.23 KB

For point 3: we need to test unnamed placeholders, because it is supported by PDO and also documented for our db_query_range():

 * @param $args
 *   An array of values to substitute into the query.  If the query uses named
 *   placeholders, this is an associative array in any order.  If the query uses
 *   unnamed placeholders (?), this is an indexed array and the order must match
 *   the order of placeholders in the query string.

Others are simplify based on suggestion :D

hswong3i’s picture

P.S. Just a little promotion: may we also give some love to its sister issue?
#322457: [DBTNG + simpletest]: queryTemporary() is buggy and need simpletest test case

hswong3i’s picture

Priority: Normal » Critical
FileSize
4.27 KB

Patch reroll via CVS HEAD. No change. Pump as critical so we will not miss it :-)

Dries’s picture

Waiting for Crell's review on this.

Crell’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

This is not critical.

- We don't need to test unnamed placeholders. Instead, we should fix the db_query_range() docs to not mention them. :-)

- As stated before, there's no need to break the query() method call into two lines; it's fine as one.

- We should include a test that doesn't start from 0 to make sure that works as well, say db_query_range(..., 1, 3), which should return 3 results.

hswong3i’s picture

Status: Needs work » Needs review
FileSize
3.77 KB

Patch revamp based on above suggestion.

hswong3i’s picture

Merge with progress of #325895: [DBTNG + simpletest] queryTemporary is broken and need simpletest test case and sync their coding style. Other improvement:

  1. DatabaseRangeQueryTestCase is now extends from DrupalWebTestCase, so performance boosted.
  2. Also check if return target data, besides check if return correct number of rows.
  3. Keep db_query_range() comments as existing. According to chx comment the remove of hits for unnamed placeholders should belongs to another issue.

Patch via CVS HEAD and simpletest with MySQL, pass.

hswong3i’s picture

Title: [DBTNG + simpletest]: queryRange() need update and simpletest test case » [DBTNG + simpletest] queryRange() need update and simpletest test case
hswong3i’s picture

Minor documentation update for getInfo()'s description.

hswong3i’s picture

Patch reroll via CVS HEAD.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

There's some offsets but still applies, looks good, and the relevant tests pass.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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