Make Postgres provider test idempotent #45418

strk · 2021-10-05T20:40:04Z

TODO:

strk · 2021-10-05T20:44:38Z

Interestingly, it takes 4 runs of TestPyQgsPostgresProvider.testNonPkBigintField to get a failure:

======================================================================
FAIL: testNonPkBigintField (__main__.TestPyQgsPostgresProvider)
Test if we can correctly insert, read and change attributes(fields) of type bigint and which are not PKs.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/src/python/test_provider_postgres.py", line 652, in testNonPkBigintField
    self.assertEqual(f.attributes()[bigint_with_default_idx], 42)
AssertionError: 43 != 42

----------------------------------------------------------------------

strk · 2021-10-05T21:15:54Z

The TestPyQgsPostgresProvider.testPktUpdateBigintPkNonFirst test for me fails on first run so I cannot confirm there's an idempotency problem there:

======================================================================
FAIL: testPktUpdateBigintPkNonFirst (__main__.TestPyQgsPostgresProvider)
Test if we can update objects with positive, zero and negative bigint PKs in tables whose PK is not the first field
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/src/python/test_provider_postgres.py", line 803, in testPktUpdateBigintPkNonFirst
    self.assertTrue(all(x == 0 for x in statuses))
AssertionError: False is not true

I'll file a separate ticket for that one, tomorrow

elpaso · 2021-10-06T07:16:56Z

I think that the best approach is to get rid of the external SQL file and drop/create/populate tables in each test like in

QGIS/tests/src/python/test_provider_postgres.py

Line 2802 in 41f1924

# Cleanup

strk · 2021-10-06T08:54:00Z

I think that the best approach is to get rid of the external SQL file and drop/create/populate tables in each test like in

QGIS/tests/src/python/test_provider_postgres.py

Line 2802 in 41f1924

# Cleanup

I'd prefer actually reverting changes while still keeping an upfront load of things, or a bug in the creation of data tables would result in failing all tests. And also to reduce the cost of running multiple tests at once (some read-only test would not need to create again and again the same tables)

strk · 2021-10-06T08:56:20Z

Interestingly, CI is getting my own failure: https://github.com/qgis/QGIS/pull/45418/checks?check_run_id=3808276429#step:13:834
but only after the commit changing testPktUpdateBigintPk, so it looks like testPktUpdateBigintPkNonFirst EXPECTS that testPktUpdateBigintPk runs first and leaves the database MODIFIED. This should be fixed too

strk · 2021-10-07T08:48:19Z

Great: the testPktUpdateBigintPkNonFirst test was actually BOGUS in that it changed values in one table and checked if they were changed in ANOTHER table (typo).

strk · 2021-10-07T10:23:57Z

Ok CI is now happy, but locally I still have failures on SECOND run in testOrderBy and testOrderByCompiled, as if some of the tests run after those, somehow change the data for next invocation of those tests. I'm trying to figure how which tests do that.

strk · 2021-10-07T10:27:48Z

It's actually the 3rd run which fails and I suspect it has to do with sequences not being reset, as the failure on 3rd run is an additional row with identifier 6:

AssertionError: Lists differ: [6, 5, 4, 3, 2, 1] != [5, 4, 3, 2, 1]

And the 4th run fails with an additional row with identifier 8:

AssertionError: Lists differ: [8, 5, 4, 3, 2, 1] != [5, 4, 3, 2, 1]

strk · 2021-10-07T10:57:01Z

The "someData_pk_seq" sequence starts as uncalled, with lastVal being 1, while the table contains values from 1 to 5.
This possibly explains why the second run still pass because there must be some test that is incrementing that sequence and things will only start failing once the lastVal reaches 5 ... (fails starting at 6 as shown above).

strk · 2021-10-07T17:36:48Z

TestPyQgsPostgresProvider.testNestedInsert is responsible to increment that sequence (at least: one of the responsibles)

strk · 2021-10-07T18:23:33Z

The testNestedInsert not only increments that sequence, but is also BROKEN in that it never succeeds at adding the feature to the layer, due to the sequence returning already-used primary keys. This is probably worth another ticket. When fixing the sequence to make it succeed in adding a new feature, the test DEADLOCKS, which was supposedly waht the test was about (preventing the deadlock)

strk · 2021-10-07T18:44:09Z

@m-kuhn I saw the test for deadlocks with transactions was yours (in 07fcf24)

You will notice that the addFeature() there fails (try asserting True there). Is that line meant to fail ? Beacuse when it succeeds (after properly initializing the sequence) I do get a deadlock...

strk · 2021-10-07T18:57:52Z

It looks like adding a commitChanges (or rollback) fixes the deadlock. Pushed.

strk · 2021-10-07T19:00:47Z

In it's current state, this PR gives me a fully idempotent TestPyQgsPostgresProvider run, fixing #45417

m-kuhn · 2021-10-07T21:23:12Z

@m-kuhn I saw the test for deadlocks with transactions was yours (in 07fcf24)

You will notice that the addFeature() there fails (try asserting True there). Is that line meant to fail ? Beacuse when it succeeds (after properly initializing the sequence) I do get a deadlock...

I don't recall the details of it, looking in retrospective the interest was to get no deadlock. Regardless if the action itself fails or succeeds.

References qgis#45417

The test was checking the *wrong* table for effects of edits performed. This commit fixes that, makes failures easier to understand and makes the test idempotent.

Use from testJson

Also have it commit changes to avoid leaving cursors open

nyalldawson · 2021-10-13T22:34:41Z

tests/src/python/test_provider_postgres.py

+    def scopedTableBackup(self, schema, table):
+
+        class ScopedBackup():
+            def __init__(self, tester, schema, table):
+                self.schema = schema
+                self.table = table
+                self.tester = tester
+                tester.execSQLCommand('DROP TABLE IF EXISTS {s}.{t}_edit CASCADE'.format(s=schema, t=table))
+                tester.execSQLCommand('CREATE TABLE {s}.{t}_edit AS SELECT * FROM {s}.{t}'.format(s=schema, t=table))
+
+            def __del__(self):
+                self.tester.execSQLCommand('TRUNCATE TABLE {s}.{t}'.format(s=self.schema, t=self.table))
+                self.tester.execSQLCommand('INSERT INTO {s}.{t} SELECT * FROM {s}.{t}_edit'.format(s=self.schema, t=self.table))
+                self.tester.execSQLCommand('DROP TABLE {s}.{t}_edit'.format(s=self.schema, t=self.table))
+
+        return ScopedBackup(self, schema, table)


I like this approach! I can't help but feel like it's only a few tweaks away from being a real context manager though. E.g. it would be cleaner/more pythonic to be able to directly call:

def my_test(self): # ... with self.backup_table('schema','my_table'): # some sql # ...

I thought about that but then some tests use more than a single table, so the with would need multiple tables to be backed up

potentially we could nest the contexts... it's not super nice though: 😛

with self.backup_table('....'): with self.backup_table('....'): ....

Or provide a list of tables?

strk self-assigned this Oct 5, 2021

github-actions bot added this to the 3.22.0 milestone Oct 5, 2021

strk changed the title ~~idempotent postgresprovider test~~ Make Postgres provider test idempotent Oct 5, 2021

strk changed the title ~~Make Postgres provider test idempotent~~ WIP: Make Postgres provider test idempotent Oct 5, 2021

strk marked this pull request as draft October 5, 2021 20:40

strk force-pushed the 45417-idempotent-postgresprovider-test branch from 36d65ab to bc63748 Compare October 5, 2021 20:42

strk force-pushed the 45417-idempotent-postgresprovider-test branch from 3cf0008 to 884e3ce Compare October 5, 2021 20:58

strk added PostGIS data provider testsuite Issue related to testsuite labels Oct 5, 2021

strk requested a review from nyalldawson October 5, 2021 21:17

strk linked an issue Oct 5, 2021 that may be closed by this pull request

TestPyQgsPostgresProvider is not idempotent #45417

Closed

strk changed the title ~~WIP: Make Postgres provider test idempotent~~ Make Postgres provider test idempotent Oct 7, 2021

strk requested a review from elpaso October 7, 2021 08:57

strk force-pushed the 45417-idempotent-postgresprovider-test branch from 3d2f017 to aedc711 Compare October 7, 2021 09:26

strk force-pushed the 45417-idempotent-postgresprovider-test branch from f992a6a to aedc711 Compare October 7, 2021 18:02

strk marked this pull request as ready for review October 7, 2021 19:00

strk mentioned this pull request Oct 7, 2021

Many PyQgsPostgresProvider tests fail with ERROR: operator does not exist: integer = text #45380

Closed

2 tasks

strk requested a review from m-kuhn October 7, 2021 22:03

strk added 11 commits October 9, 2021 14:45

Make TestPyQgsPostgresProvider.testGeneratedFields idempotent

5510ae4

References qgis#45417

Make TestPyQgsPostgresProvider.testNonPkBigintField idempotent

9a5f08f

References qgis#45417

Make TestPyQgsPostgresProvider.testPktComposite idempotent

f55fc5e

References qgis#45417

Make TestPyQgsPostgresProvider.testPktCompositeFloat idempotent

1159f3c

References qgis#45417

Make TestPyQgsPostgresProvider.testPktFloatingPoint idempotent

b569434

References qgis#45417

Make TestPyQgsPostgresProvider.testPktUpdateBigintPk idempotent

0ee9969

References qgis#45417

Fix testPktUpdateBigintPkNonFirst test and make idempotent

5b20ab9

The test was checking the *wrong* table for effects of edits performed. This commit fixes that, makes failures easier to understand and makes the test idempotent.

Use an exception-safe TableBackup class for reentrancy

32e68a1

Use from testJson

Expose and use a simpler scopedTableBackup() method

da3ee48

Initialize the sequence associated with serial PK of editable layer

80829ba

Have testNestedInsert use an editable layer, making it idempotent

8dbc7d2

Also have it commit changes to avoid leaving cursors open

strk force-pushed the 45417-idempotent-postgresprovider-test branch from 96902ad to 8dbc7d2 Compare October 9, 2021 12:46

strk requested a review from timlinux October 12, 2021 14:03

strk mentioned this pull request Oct 13, 2021

TestPyQgsPostgresProvider is not idempotent #45417

Closed

nyalldawson reviewed Oct 13, 2021

View reviewed changes

nyalldawson approved these changes Oct 13, 2021

View reviewed changes

strk merged commit 51d728b into qgis:master Oct 13, 2021

strk deleted the 45417-idempotent-postgresprovider-test branch October 13, 2021 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Postgres provider test idempotent #45418

Make Postgres provider test idempotent #45418

strk commented Oct 5, 2021 •

edited

Loading

strk commented Oct 5, 2021

strk commented Oct 5, 2021

elpaso commented Oct 6, 2021

strk commented Oct 6, 2021

strk commented Oct 6, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

m-kuhn commented Oct 7, 2021

nyalldawson Oct 13, 2021

strk Oct 13, 2021

nyalldawson Oct 13, 2021

m-kuhn Oct 14, 2021

Make Postgres provider test idempotent #45418

Make Postgres provider test idempotent #45418

Conversation

strk commented Oct 5, 2021 • edited Loading

strk commented Oct 5, 2021

strk commented Oct 5, 2021

elpaso commented Oct 6, 2021

strk commented Oct 6, 2021

strk commented Oct 6, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

strk commented Oct 7, 2021

m-kuhn commented Oct 7, 2021

nyalldawson Oct 13, 2021

Choose a reason for hiding this comment

strk Oct 13, 2021

Choose a reason for hiding this comment

nyalldawson Oct 13, 2021

Choose a reason for hiding this comment

m-kuhn Oct 14, 2021

Choose a reason for hiding this comment

strk commented Oct 5, 2021 •

edited

Loading