Roundup Tracker - Issues

Issue 2550910

classification
Title: tests/test_indexer.py does not run if you are missing any back ends.
Type: behavior Severity: major
Components: Test Versions: devel, 1.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: jerrykan Nosy List: jerrykan, rouilj
Priority: normal Keywords:

Created on 2016-06-26 00:17 by rouilj, last changed 2016-07-01 01:51 by jerrykan.

Messages
msg5626 Author: [hidden] (rouilj) Date: 2016-06-26 00:17
When testing patch for issue2550636, it turns out that none of
the tests in test_indexer.py would run. I don't know what is wrong
but I worked around it by commenting out the import and classes
for the backends that I did not have installed.

I poked at it trying to fix/diagnose the decorators used to prevent
the tests for the missing components from running, but my python-fu
isn't up to it.

My environment was: linux mint 17.3

> ./run_tests.py --debug -r a --verbose --capture=sys test/test_indexer.py
writing pytestdebug information to
/home/rouilj/develop/roundup.dev/roundup.hg/pytestdebug.log
============================= test session starts
==============================
platform linux2 -- Python 2.7.6, pytest-2.8.4, py-1.4.30, pluggy-0.3.1
-- /usr/bin/python
using: pytest-2.8.4 pylib-1.4.30
cachedir: .cache
rootdir: /home/rouilj/develop/roundup.dev/roundup.hg, inifile: 
collected 0 items / 1 skipped 

=========================== short test summary info
============================
SKIP [1]
/home/rouilj/develop/roundup.dev/roundup.hg/test/test_mysql.py:53:
Skipping MySQL tests: (2002, "Can't connect to local MySQL server
through socket '/var/run/mysqld/mysqld.sock' (2)")
========================== 1 skipped in 3.36 seconds
===========================
wrote pytestdebug information to
/home/rouilj/develop/roundup.dev/roundup.hg/pytestdebug.log
msg5629 Author: [hidden] (jerrykan) Date: 2016-06-26 13:07
Yes, I confirm this bug. I believe this is related to an open pytest issue:

  https://github.com/pytest-dev/pytest/issues/568

I thought I had worked-around the issue and tested it, but apparently not.

The pytest issue contains a reference to the following work-around:

 
https://github.com/Parsely/pykafka/blob/e18bdeb3586fab21493b755d564d6b4aa8ec7ef2/tests/pykafka/__init__.py#L4

I've had a quick look and it seems to work, so I'll look into preparing
a patch for the roundup tests to use until the bug is fixed in pytest.
msg5630 Author: [hidden] (jerrykan) Date: 2016-06-26 14:50
On closer inspection the solution used by pykafka may not work because
of roundup's use of multiple inheritance for is DB tests.

I also noticed that the the tests are currently using 'pytest.skip'
instead of 'pytest.marker.skip', which probably doesn't help.

I'll keep doing a bit more work to see if I can find a suitable workaround.
msg5662 Author: [hidden] (jerrykan) Date: 2016-06-27 04:46
Work-around for pytest marker bug related to skipping tests fixed in commit:

  https://sourceforge.net/p/roundup/code/ci/37d1e24fb9

run_tests.py updated to latest pytest version to fix bug with
'pytest.marker.skip' not correctly skipping tests in commit:

  https://sourceforge.net/p/roundup/code/ci/07168ae926

I believe this issue can be closed for now, unless you are still
experiencing problems John?
msg5664 Author: [hidden] (rouilj) Date: 2016-06-27 22:26
Well it's better but postgres tests still being run.

I just hg pulled and hg updated. hg diff reports no differences.

Running:

 ./run_test --verbose test/test_indexer.py gives me:

test/test_indexer.py::XapianIndexerTest::test_stopwords PASSED
test/test_indexer.py::XapianIndexerTest::test_wordsplitting PASSED
test/test_indexer.py::postgresqlIndexerTest::test_basics FAILED
test/test_indexer.py::postgresqlIndexerTest::test_casesensitity FAILED
test/test_indexer.py::postgresqlIndexerTest::test_change FAILED
test/test_indexer.py::postgresqlIndexerTest::test_clear FAILED
test/test_indexer.py::postgresqlIndexerTest::test_extremewords FAILED
test/test_indexer.py::postgresqlIndexerTest::test_manyresults FAILED
test/test_indexer.py::postgresqlIndexerTest::test_stopwords FAILED
test/test_indexer.py::postgresqlIndexerTest::test_wordsplitting FAILED
test/test_indexer.py::mysqlIndexerTest::test_basics SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_casesensitity SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_change SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_clear SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_extremewords SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_manyresults SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_stopwords SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_wordsplitting SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_basics SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_casesensitity SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_change SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_clear SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_extremewords SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_manyresults SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_stopwords SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_wordsplitting SKIPPED

sqlite shouldn't be skipped. Postgres should be skipped not fail.

All the postgres errors are of the form:

>           raise hyperdb.DatabaseError(message)
E           DatabaseError: could not connect to server: Connection refused
E           	Is the server running on host "localhost" (127.0.0.1) and
accepting
E           	TCP/IP connections on port 5432?

roundup/backends/back_postgresql.py:91: DatabaseError

If I move the definiton do the sqlite test before postgres and mysql,
they all pass. So it looks like the @skip_mysql decorator is
applying to the

class sqliteIndexerTest(sqliteOpener, RDBMSIndexerTest, IndexerTest):
    pass

unless I move this class definition above postgres.

If I reorder some more and move mysql above postgres I get:

[...]
test/test_indexer.py::sqliteIndexerTest::test_stopwords PASSED
test/test_indexer.py::sqliteIndexerTest::test_wordsplitting PASSED
test/test_indexer.py::mysqlIndexerTest::test_basics SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_casesensitity SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_change SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_clear SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_extremewords SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_manyresults SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_stopwords SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_wordsplitting SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_basics SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_casesensitity SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_change SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_clear SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_extremewords SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_manyresults SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_stopwords SKIPPED
test/test_indexer.py::postgresqlIndexerTest::test_wordsplitting SKIPPED

So not sure what to do here. Ideas?

-- rouilj
msg5674 Author: [hidden] (jerrykan) Date: 2016-06-28 05:22
On 28/06/16 08:26, John Rouillard wrote:
> Well it's better but postgres tests still being run.

Check to see if you have the 'psycopg' python package installed. The 
tests rely on using 'roundup.backend.have_backend()'[1] to determine if 
the backend is available and will attempt to run the tests if it returns 
True. The 'have_backend()' essentially returns True for postgresql if 
the 'psycopg' package is installed.

> sqlite shouldn't be skipped. Postgres should be skipped not fail.
>
> All the postgres errors are of the form:
>
>>           raise hyperdb.DatabaseError(message)
> E           DatabaseError: could not connect to server: Connection refused
> E           	Is the server running on host "localhost" (127.0.0.1) and
> accepting
> E           	TCP/IP connections on port 5432?
>
> roundup/backends/back_postgresql.py:91: DatabaseError
>
> If I move the definiton do the sqlite test before postgres and mysql,
> they all pass. So it looks like the @skip_mysql decorator is
> applying to the
>
> class sqliteIndexerTest(sqliteOpener, RDBMSIndexerTest, IndexerTest):
>     pass
>
> unless I move this class definition above postgres.

It seems as though 'pytest.mark.skip()' bug[2] taints any test that 
shares a parent test class, not just the other tests that are marked to 
be skipped by pytest - which is what I thought the problem was.

I'll do a bit more looking to see if an improved work-around can be found.

[1] 
https://sourceforge.net/p/roundup/code/ci/37d1e24fb9/tree/roundup/backends/__init__.py#l46

[2] https://github.com/pytest-dev/pytest/issues/568
msg5675 Author: [hidden] (jerrykan) Date: 2016-06-28 06:15
OK, another attempt at fixing this has been committed in:

  https://sourceforge.net/p/roundup/code/ci/43a1f7fe39/
msg5678 Author: [hidden] (rouilj) Date: 2016-06-28 12:08
Hi John:

In message <e741293b-947c-0f5e-6774-e0df1ae25452@jerrykan.com>,
John Kristensen writes:
>John Kristensen added the comment:
>On 28/06/16 08:26, John Rouillard wrote:
>> Well it's better but postgres tests still being run.
>
>Check to see if you have the 'psycopg' python package installed. The 
>tests rely on using 'roundup.backend.have_backend()'[1] to determine if 
>the backend is available and will attempt to run the tests if it returns 
>True. The 'have_backend()' essentially returns True for postgresql if 
>the 'psycopg' package is installed.

Hmm, mysql IIRC tries to conenct to the db as part of have_backend()
or some wrapper around have_backend() in the tests. If it can't
connect it skips.

I'll see if I can figure out how mysql handles this as I have both
libraries installed just not the databases.
>> [elided tests being skipped that shouldn't be.]
>It seems as though 'pytest.mark.skip()' bug[2] taints any test that 
>shares a parent test class, not just the other tests that are marked to 
>be skipped by pytest - which is what I thought the problem was.
>
>I'll do a bit more looking to see if an improved work-around can be found.

Thanks for trying to track this down.
msg5679 Author: [hidden] (rouilj) Date: 2016-06-28 21:22
Hi Jerry:

Your latest patch sees to have done much of the trick.
...
test/test_indexer.py::XapianIndexerTest::test_manyresults PASSED
test/test_indexer.py::XapianIndexerTest::test_stopwords PASSED
test/test_indexer.py::XapianIndexerTest::test_wordsplitting PASSED
test/test_indexer.py::postgresqlIndexerTest::test_basics FAILED
test/test_indexer.py::postgresqlIndexerTest::test_casesensitity FAILED
test/test_indexer.py::postgresqlIndexerTest::test_change FAILED
test/test_indexer.py::postgresqlIndexerTest::test_clear FAILED
test/test_indexer.py::postgresqlIndexerTest::test_extremewords FAILED
test/test_indexer.py::postgresqlIndexerTest::test_manyresults FAILED
test/test_indexer.py::postgresqlIndexerTest::test_stopwords FAILED
test/test_indexer.py::postgresqlIndexerTest::test_wordsplitting FAILED
test/test_indexer.py::mysqlIndexerTest::test_basics SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_casesensitity SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_change SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_clear SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_extremewords SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_manyresults SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_stopwords SKIPPED
test/test_indexer.py::mysqlIndexerTest::test_wordsplitting SKIPPED
test/test_indexer.py::sqliteIndexerTest::test_basics PASSED
test/test_indexer.py::sqliteIndexerTest::test_casesensitity PASSED
test/test_indexer.py::sqliteIndexerTest::test_change PASSED
...

I'll need to figure out how mysql gets skipped but postgres doesn't
as import MySQLdb works fine and the backend check seems to pass
but mysql tests are still properly skipped.

Thanks.
msg5684 Author: [hidden] (rouilj) Date: 2016-06-29 22:07
Sigh, John looks like your fix broke something else.
I just ran a full test before checkin in some additional tests I
developed and I see:

test/test_mysql.py::mysqlSchemaTest::test_removeClassKey <-
test/db_test_base.py SKIPPED
test/test_mysql.py::mysqlSchemaTest::test_removeMultilink <-
test/db_test_base.py SKIPPED
test/test_mysql.py::mysqlSchemaTest::test_reservedProperties <-
test/db_test_base.py SKIPPED
test/test_mysql.py::mysqlClassicInitTest::testCreation <-
test/db_test_base.py FAILED
test/test_mysql.py::mysqlConcurrencyTest::testConcurrency <-
test/db_test_base.py FAILED

I don't have mysql running, and for a while it properly skips things
then .....

Are you running a full test suite when trying to validate
run_tests, or were you following my lead and only running the
indexer stuff?
msg5686 Author: [hidden] (jerrykan) Date: 2016-06-30 03:49
On 30/06/16 08:07, John Rouillard wrote:
> Sigh, John looks like your fix broke something else.
Argh. Serves me right for trying to push out some quick fixes. I've 
taken a bit of time to look at the problem properly and this time it was 
because I didn't look closely enough at the mark_class decorator when I 
copy and pasted it.

Details of the fix can be found in the commit message rev1c94afabb2cb

This time I promise everything should be in working order*

(* promise comes with no guarantee)

> Are you running a full test suite when trying to validate
> run_tests, or were you following my lead and only running the
> indexer stuff?

I run the full test suite using TravisCI before I push back my changes 
upstream, but that tests everything and doesn't skip anything, which is 
why I didn't pick up this problem. I also tested just test_indexer.py 
which didn't exhibit the problems we were hitting.

I've done a bit more manual testing of the tests this time, so hopefully 
it will all be good now.
msg5687 Author: [hidden] (jerrykan) Date: 2016-06-30 04:00
On 28/06/16 22:08, John Rouillard wrote:
> Hmm, mysql IIRC tries to conenct to the db as part of have_backend()
> or some wrapper around have_backend() in the tests. If it can't
> connect it skips.

The bit of code you are interested in is at:

https://sourceforge.net/p/roundup/code/ci/1c94afabb2/tree/test/test_mysql.py#l47

which in addition to checking id the backend exists, attempts to check 
if the database exists as well, using:

https://sourceforge.net/p/roundup/code/ci/1c94afabb2/tree/roundup/backends/back_mysql.py#l98

I am not entirely convinced that this extra checking is desirable. If 
the database does not exist I would prefer that the tests fail loudly 
rather than skip silently (and potentially go unnoticed).

So my opinion would be that the logic used to determine if the the mysql 
tests are skipped should be the same as postgresql, not vice-versa.

Maybe this discussion should happen in another issue? or on the mailing 
list so others can add their opinion?
msg5689 Author: [hidden] (rouilj) Date: 2016-06-30 17:02
Hi John:

In message <7867032f-878b-43ed-21fe-3829e7a1b349@jerrykan.com>,
John Kristensen writes:
>John Kristensen added the comment:
>
>On 28/06/16 22:08, John Rouillard wrote:
>> Hmm, mysql IIRC tries to conenct to the db as part of have_backend()
>> or some wrapper around have_backend() in the tests. If it can't
>> connect it skips.
>The bit of code you are interested in is at:
>[...]
>which in addition to checking id the backend exists, attempts to check 
>if the database exists as well, using:
>[...]
>I am not entirely convinced that this extra checking is desirable. If 
>the database does not exist I would prefer that the tests fail loudly 
>rather than skip silently (and potentially go unnoticed).

Except that simply having the backend python libraries in place on
a system doesn't mean you have configured a database.

For example, if I am running roundup using mysql on a system that also
run some other app using postgres, and I want to test the code before
install, it will falsely fail postgres tests possibly obscuring other
failing tests that I care about.

Right now running tests with an explicit -k 'not (mysql or
postgresql)' is required. But is that guaranteed to only disable tests
that require a running copy of those two databases? Could it
accidently trap other tests?

>So my opinion would be that the logic used to determine if the the mysql 
>tests are skipped should be the same as postgresql, not vice-versa.

I see your case, but would argue for the opposite. Would this work?

Add an explicit "is database up" test designed to catch the case where
the back end is configured but there is no db backing it?

So the skip decorators will disable all mysql/postgres tests if the
backend is missing or if there is no db to connect to. However the new
PostgresIsDatabaseUp and MysqlIsDatabaseUp tests would *not* be
decorated. This way the two errors are reported if -k 'not
IsDatabaseUp' (a superset of -k PostgresIsDatabaseUp and -k
MysqlIsDatabaseUp) isn't used.

Does that fill your requirement?

>Maybe this discussion should happen in another issue? or on the mailing 
>list so others can add their opinion?

If you think my proposal needs more thought, or you want to show it to
a larger audience, feel free to toss this issue and msg references on
the list. We can open a new issue if/when this is ready for
implementation.
msg5693 Author: [hidden] (jerrykan) Date: 2016-07-01 01:51
On 01/07/16 03:02, John Rouillard wrote:
>> John Kristensen added the comment:
>>
>> I am not entirely convinced that this extra checking is desirable. If
>> the database does not exist I would prefer that the tests fail loudly
>> rather than skip silently (and potentially go unnoticed).
>
> Except that simply having the backend python libraries in place on
> a system doesn't mean you have configured a database.
>
> For example, if I am running roundup using mysql on a system that also
> run some other app using postgres, and I want to test the code before
> install, it will falsely fail postgres tests possibly obscuring other
> failing tests that I care about.

My counter argument to this would be that each application should be 
running in its own virtualenv. Once two applications are relying on a 
common dependency it is only a matter of time before it comes back to 
bite you with version incompatibilities.

> Right now running tests with an explicit -k 'not (mysql or
> postgresql)' is required. But is that guaranteed to only disable tests
> that require a running copy of those two databases? Could it
> accidently trap other tests?

I'll say "maybe". You would probably need to just check which test names 
are matched and hope the naming of those, or new, tests stay consistent 
in the future.

An alternative may be to look at using marks to make this sort of thing 
easier:

   https://pytest.org/latest/example/markers.html

Though given the issues I've had so far with the skip markers, I'm a 
little hesitant to try and implement them and break everything again ;)
History
Date User Action Args
2016-07-01 01:51:16jerrykansetmessages: + msg5693
2016-06-30 17:02:15rouiljsetmessages: + msg5689
2016-06-30 04:00:23jerrykansetmessages: + msg5687
2016-06-30 03:49:17jerrykansetmessages: + msg5686
2016-06-29 22:07:59rouiljsetmessages: + msg5684
2016-06-28 21:22:06rouiljsetmessages: + msg5679
2016-06-28 12:08:21rouiljsetmessages: + msg5678
2016-06-28 06:15:15jerrykansetmessages: + msg5675
2016-06-28 05:22:29jerrykansetmessages: + msg5674
2016-06-27 22:26:17rouiljsetmessages: + msg5664
2016-06-27 04:46:38jerrykansetstatus: new -> open
messages: + msg5662
2016-06-26 14:50:57jerrykansetmessages: + msg5630
2016-06-26 13:07:48jerrykansetassignee: jerrykan
messages: + msg5629
nosy: + jerrykan
2016-06-26 00:17:04rouiljcreate