Starting a test framework

[dverdin]

I added a dummy (and empty) SQLite database and a dummy sympa.conf file.
These are used in a test file for Conf.pm and another one for Sympa::DatabaseManager.
These two modules should be tested first (hence the number for the test files). Providing everything works, we can start testing other files, being sure that they will be allowed to access our dummy database and configuration.
Of course, it is just a start: still more work to populate the database, being able to empy it and repopulate it easily, and also using ll configs: auth.conf, lists configs, etc.

[ikedas]

I don't know the best way, but use of Conf::load() seems inconvenient: We have to prepare different sympa.conf by each test.

One idea is defining %Conf::Conf with required parameters by each test:

%Conf::Conf = (
    domain     => 'lists.example.com',  # mandatory
    listmaster => 'dude@example.com',   # mandatory
    lang       => 'en-US',
    db_type    => 'SQLite',
    db_name    => 't/dummy_data/sympa',
);

[dverdin]

Good idea! It has the added value of restricting configuration for each module to what is strictly necessary.
Except for Conf.pm test, of course... ;-)
I'll add it to the PR.

[dverdin]

Done!

[racke]

Data in t/ directory is by definition dummy data, so please rename the directory to data or something equally appropriate.

[dverdin]

Nitpicker... ;-)
OK, I'll do it.

[racke]

Conf.t fails when I run it from my repo. Also if you have no compelling reason for using numbers in the test file names, please drop them.

racke@belukha:~/sympa-community/sympa$ prove -vl t/01_Conf.t 
t/01_Conf.t .. 
ok 1 - use Conf;
notice Conf::_load_auth() Unable to open /home/sympa/default/auth.conf: No such file or directory
err Conf::get_robots_list() Unable to open directory /home/sympa/etc for virtual robots config
err Conf::load_robots() Robots config loading failed
not ok 2 - Configuration file loading
#   Failed test 'Configuration file loading'
#   at t/01_Conf.t line 18.
ok 3 - Check correct loading of Sympa gecos parameter
1..3
# Looks like you failed 1 test of 3.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 

Test Summary Report
-------------------
t/01_Conf.t (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=3,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.24 cusr  0.01 csys =  0.26 CPU)
Result: FAIL

[dverdin]

Yes I have reasons for using numbers: I want some modules to be tested first. Conf.pm is everywhere, so any bug must be spotted first. Seeing it in error at the top of the list will help debugging process.

[dverdin]

OK. I generated the config file on server on which Sympa is installed. I'l remove it and test it blankly.
Fix coming soon!

[dverdin]

It needed an etc/ directory to work. I added it empty and now it's working.

[racke]

There is no reason for adding an arbitrary file for keeping a directory in Git: https://wiki.linuxia.de/library/stefan-hornburg-racke-git-cheatsheet-en#text-amuse-label-emptyfolder

[racke]

Ok, the test is now successful 👍

It would be nice though to get rid of the following Sympa message:

notice Conf::_load_auth() Unable to open /home/sympa/default/auth.conf: No such file or directory

[dverdin]

About .gikeep. I found the follwing argument convincing (prevent confusion with actual gitignore) : https://stackoverflow.com/questions/7229885/what-are-the-differences-between-gitignore-and-gitkeep/7229996#7229996

[dverdin]

About the message : it is not in the test, ins't it? Only in Sympa logs?

[dverdin]

About the overall test success : Cool! ;-)

[racke]

This argument doesn't convince me. In my opinion it is better to use existing techniques (.gitignore) instead of inventing workarounds (kind of like poor man's backups). Please change.

[racke]

racke@belukha:~/sympa-community/sympa$ prove -vl t/01_Conf.t 
t/01_Conf.t .. 
ok 1 - use Conf;
notice Conf::_load_auth() Unable to open /home/sympa/default/auth.conf: No such file or directory
ok 2 - Configuration file loading
ok 3 - Check correct loading of Sympa gecos parameter
1..3
ok
All tests successful.
Files=1, Tests=3,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.22 cusr  0.02 csys =  0.26 CPU)
Result: PASS

[dverdin]

Weird:

t/01_Conf.t .. 
ok 1 - use Conf;
ok 2 - Configuration file loading
ok 3 - Check correct loading of Sympa gecos parameter
1..3
ok
All tests successful.
Files=1, Tests=3,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.32 cusr  0.01 csys =  0.35 CPU)
Result: PASS

[dverdin]

I'll add a dummy auth.conf file anyway (which also solves the .gitkeep issue ;-) )

[dverdin]

But I'll use .gitignore if you'd rather see this.
That should be in the contribution guidelines, by the way.

[dverdin]

So I added the auth.conf file. This is not an actual fix of Sympa code, but as it is not the purpose of the current PR, I won't touch anything in Sympa code. Only in tests.

[racke]

Good 👍

[dverdin]

Cool!
So... Shall we merge?

[racke]

@ikedas What do you think?

[ikedas]

I’ll check tomorrow.
Would these files be added to Makefile.am?

[dverdin]

@ikedas : They probably should, as all other test files are in Makefile.am too. Do you want me to add them?
Good night!

[ikedas]

I checked with:

• Perl 5.24.1
• DBD::SQLite 1.58 [SQLite 3.22.0].

t/01_Conf.t

• As 'SYMPA' is default value of gecos, test succeeds even if t/data/sympa.conf didn't contain gecos line, i.e. Conf::load() didn't load it. Using mandatory parameter domain or listmaster looks better.
  [Addition] Or check both parameters with and without default.

t/02_Sympa_DatabaseManager.t

• I feel name of database file would have distinctive extension like sympa.sqlite.
• Empty file (zero length) works as database file, and we may not have to bundle file with content.
  • We'd be better to create database content at the time of testing DDL/DML.

---

@ikedas : They probably should, as all other test files are in Makefile.am too. Do you want me to add them?

Yes please. they are occasionally missed, and discovered during release work. 😉

[dverdin]

Hi @ikedas , and thanks for taking the time to review my work!
I agree for the database. Actually, that should be my next step: create a script to populate / empty the database. Actually, dropping full content and populating the database should be done at the start of each test file using the database. That way, we would be sure of the test database state when running any test. AND we would not have to ship a binary file.
However, I wanted to integrate the current proof of concept first (we can use a local database for tests). Then only would I tackle the question of the database creation / destruction at test time.

Agreed for the database file name. I'll change it to sympa.sqlite.

Agreed also for the parameter. Indeed, basing a test on a parameter with a default hard-coded value was kind of dumb from me...

I'll also update the Makefile.am.

Regards,

David

[dverdin]

A little addition: I will create a test file in the xt/ directory to test the script used to populate the test database... Looks like the good place to do it.
Obviously, using DBIx::Class would be the easiest way to create such script, but I feel like adding this dependency right now is not the best move. Running an SQL query should be enough.

[dverdin]

Fuck. The Travis CI fails. I'll check what's wrong and fix.

[dverdin]

Weird. Everything is working like a charm at home.
With Perl 5.22.1.
I ran the command that is claimed to be failing and it works... I don't get what's wrong.
I'll try to completely checkout and retry.

[dverdin]

Yay, it's fixed!
Baring any other comments, a merge would be cool: I could start working on a User.pm test file to - finally - fix the "password spontaneous reinitialization" bug. ;-)