testconf.php.dist for more flexible local cross db tests

[peterdd]

tests/test-*.php files can include an existing testconf.php or default testconf.php.dist for looping their tests through a list of configured database connections. As there are many database types supported by ADOdb and nobody has all databases and versions available on local test environments, this allows at least configuring individual subsets to be tested.

Well, the tests/test-* must be adapted too.

[peterdd]

example usage (for instance by tests/test-xmlschema.php):

$testdbs=array();
if(file_exists('testconf.php')){
        # your own local config 
        include('testconf.php');
} else{
        # at least run the stuff that can be always tested (sqlite3 in-memory for instance)
        include('testconf.php.dist');
}

foreach ($testdbs as $dbt){
  # some shell colors
  echo  "\033[32m".$dbt['type'].' '.$dbt['param1'].' '.$dbt['param4']."\n\033[0m"; 
  $db = ADONewConnection( $dbt['type'] );
  $db->connect(
                $dbt['param1'], 
                $dbt['param2'],
                $dbt['param3'],
                $dbt['param4'] ) || die("failed connect to ".$dbt['type']." database.");

  ... your tests using the current connection

}

[mnewnham]

This is fine, no impact on production systems

[dregad]

That's just a config file that is currently not referenced anywhere, so while it's true that it has no produciton impact, the PR does not add any value as it stands.

I am fine with the concept of having a test config and a .dist one to configure the tests, but if the actual tests do not make use of it, there is no point in merging this.

In addition, the code should follow coding guidelines (PSR2) and the $testdbs array elements should have meaningful, descriptive names (i.e. user, password, hostname, etc. instead of paramN).

[peterdd]

So how to break the hen-egg problem? :-)

The tests/* and examples on adodb.org are scattered with db setups that probably only Lohn Lim used on his system (I do not know) and aren't testable for everyone.

I made them so each paramX align a bit vertically to see also differences between database connection parameters between database types.

Also the naming of the params follows
https://adodb.org/dokuwiki/doku.php?id=v5:reference:connection:connect and
https://adodb.org/dokuwiki/doku.php?id=v5:database:connection_matrix
Their meaning differs slightly between databases.

[peterdd]

@dregad What of 'not following PSR2' is bugging you here?

The tests/* stuff is nearly unusable for anybody in current state and not included in releases nor master snapshots.

usage of such conf file could be used by testdatabases.inc.php etc when doing also required cleanup there.

[dregad]

@peterdd The more important remarks in my earlier review were

• if the actual tests do not make use of it, there is no point in merging this.

• array elements should have meaningful, descriptive names (i.e. user, password, hostname, etc. instead of paramN).

  Also the naming of the params follows
  https://adodb.org/dokuwiki/doku.php?id=v5:reference:connection:connect and
  https://adodb.org/dokuwiki/doku.php?id=v5:database:connection_matrix
  Their meaning differs slightly between databases.

  That's true, but only P1 is occasionally changing (host/service name or DSN); others are fixed: P2 user, P3 password and P4 database, schema or instance name

Please note that I'm not challenging the fact that John Lim's legacy scripts are pretty much useless today, and I am definitely not opposed to implementing proper tests or your approach of defining a set of configs. All I'm saying is that it does not make sense to merge this, until there are test scripts actually making use of it.

Now since you seem to be more interested in the details, here it goes:

What of 'not following PSR2' is bugging you here?

PSR-2 has been deprecated, so nowadays the reference should be PSR-12 (or even PER Coding Style 2.0, but to my knowledge that is not yet covered by PHP_CodeSniffer rules).

As to what I meant in terms of config guidelines:

$ phpcs --standard=PSR12 --extensions=dist /mnt/c/Temp/testconf.php.dist

FILE: /tmp/testconf.php.dist
-------------------------------------------------------------------------
FOUND 14 ERRORS AND 3 WARNINGS AFFECTING 4 LINES
-------------------------------------------------------------------------
  4 | WARNING | [ ] Line exceeds 120 characters; contains 125 characters
  8 | ERROR   | [x] Whitespace found at end of line
 10 | WARNING | [ ] Line exceeds 120 characters; contains 122 characters
 11 | ERROR   | [x] Expected at least 1 space before "="; 0 found
 11 | ERROR   | [x] Expected at least 1 space after "="; 0 found
 11 | ERROR   | [x] Expected at least 1 space before "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space after "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space before "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space after "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space before "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space after "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space before "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space after "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space before "=>"; 0 found
 11 | ERROR   | [x] Expected at least 1 space after "=>"; 0 found
 11 | WARNING | [ ] Line exceeds 120 characters; contains 141 characters
 11 | ERROR   | [x] Whitespace found at end of line
-------------------------------------------------------------------------
PHPCBF CAN FIX THE 14 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------

And of course the same changes should be applied to commented-out sample lines as well.

Long lines should be split. In this case I would recommend to declare the testdbs array entries with one key per line.

So how to break the hen-egg problem? :-)

Send a pull request that also provides at least one test case / script that makes use of your config file.

[peterdd]

Thx @dregad

Ok, I try.

[peterdd]

Have you an idea how to name tests?

I thought something like

test_0001-GH685-reserved-keyword-fieldnames.php
test_0002-GH256-datadict-table-comments.php
test_0003-SFbugs119-multi-recordset.php 
...

I sometimes see in other projects something like 001.php,002.php ..., but having a bit context at the test filename would be cool IMHO.