Run tests against PostgreSQL, fix compatibility #170

adzialocha · 2022-06-22T15:17:17Z

Add CI checking tests against PostgreSQL database 🟢
Make all SQL statements compatible with SQLite and PostgreSQL
- The queries we needed to change are commented in this PR
Introduces TestDatabaseRunner which is now used in all tests to make sure the database gets disconnected after every succeeding or failing test. It changes our style of making tests slightly, but not too much:
- Test fn is not async
- No #[tokio::test] attribute used, we build our own tokio runtime
- Test is executed within the new TestDatabaseRunner::with_db_teardown method which also gives you access to the underlying TestDatabase instance
Removes MySQL support, its complicated and we're not sure why we need it 🐈
Fixes runtime of sqlx by switching it to tokio (it was still set to async_std and probably forgotten after the switch)
Fixes a bunch of formatting and refactors some tests which didn't make use of the new fixtures yet

Closing: #31 and #98

📋 Checklist

Add tests that cover your changes
Add this PR to the Unreleased section in CHANGELOG.md
Link this PR to any issues it closes
New files contain a SPDX license header

codecov · 2022-06-22T15:29:11Z

Codecov Report

Merging #170 (5369d26) into development (5a1a845) will increase coverage by 0.65%.
The diff coverage is 99.31%.

@@               Coverage Diff               @@
##           development     #170      +/-   ##
===============================================
+ Coverage        90.01%   90.66%   +0.65%     
===============================================
  Files               43       43              
  Lines             2633     2807     +174     
===============================================
+ Hits              2370     2545     +175     
+ Misses             263      262       -1

Impacted Files	Coverage Δ
aquadoggo/src/config.rs	`6.25% <ø> (ø)`
aquadoggo/src/db/mod.rs	`83.33% <ø> (-1.29%)`	⬇️
aquadoggo/src/http/service.rs	`91.30% <92.30%> (-3.94%)`	⬇️
aquadoggo/src/db/stores/test_utils.rs	`97.65% <93.93%> (-1.33%)`	⬇️
aquadoggo/src/db/stores/operation.rs	`91.15% <95.23%> (+0.07%)`	⬆️
aquadoggo/src/graphql/client/query.rs	`95.12% <97.05%> (+1.18%)`	⬆️
aquadoggo/src/graphql/client/mutation.rs	`98.43% <99.42%> (+0.38%)`	⬆️
aquadoggo/src/db/stores/document.rs	`94.02% <100.00%> (+0.56%)`	⬆️
aquadoggo/src/db/stores/entry.rs	`95.95% <100.00%> (+0.06%)`	⬆️
aquadoggo/src/db/stores/log.rs	`97.36% <100.00%> (-0.54%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a1a845...5369d26. Read the comment docs.

…onger try-and-error phase

adzialocha · 2022-06-23T11:18:11Z

aquadoggo/src/db/mod.rs

@@ -21,8 +21,6 @@ pub async fn create_database(url: &str) -> Result<()> {
        Any::create_database(url).await?;
    }

-    Any::drop_database(url);


What was that? 😱

I think it was never await-ed, therefore it also never worked, but 😱

aquadoggo/src/db/stores/document.rs

adzialocha · 2022-06-23T11:19:46Z

aquadoggo/src/db/stores/operation.rs

@@ -144,7 +144,7 @@ impl OperationStore<VerifiedOperation> for SqlStorage {
                            .bind(name.to_owned())
                            .bind(value.field_type().to_string())
                            .bind(db_value)
-                            .bind(index.to_string())
+                            .bind(index as i32)


👆 PostgreSQL was complaining that this column is actually a number (NUMERIC).

adzialocha · 2022-06-23T11:20:52Z

aquadoggo/migrations/20220617115933_create-tasks.sql

-    COALESCE(document_view_id, 0)
-);
+-- @TODO: This fails using PostgreSQL
+-- CREATE UNIQUE INDEX ux_tasks ON tasks (


👆 Upsi, this doesn't seem to work on PostgreSQL 😢

Since the migrations fail all tests fail for PostgreSQL and MySQL currently

Fixed now via:

CREATE UNIQUE INDEX ux_tasks ON tasks ( name, COALESCE(document_id, '0'), COALESCE(document_view_id, '0') );

adzialocha · 2022-06-23T11:21:12Z

aquadoggo/migrations/20220509090252_create-operations.sql

@@ -15,7 +15,7 @@ CREATE TABLE IF NOT EXISTS operation_fields_v1 (
    operation_id            TEXT             NOT NULL,
    name                    TEXT             NOT NULL,
    field_type              TEXT             NOT NULL,
-    value                   BLOB             NULL,
+    value                   TEXT             NULL,


👆 BLOB is not known for PostgreSQL?

adzialocha · 2022-06-23T11:33:58Z

Small report

Problems of testing a database

So far this "somewhat" does what it promises: It runs the tests against a PostgreSQL and MySQL database. Problem is, that whenever a test panics it leaves the connection to the database alive, nothing gets disconnected afterwards 😢 Since we drop and re-create the database for every test this leads to ugly errors:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Database(PgDatabaseError { severity: Error, code: "55006", message: "database \"aquadoggo-development\" is being accessed by other users", detail: Some("There are 13 other sessions using the database."), hint: None, position: None, where: None, schema: None, table: None, column: None, data_type: None, constraint: None, file: Some("dbcommands.c"), line: Some(872), routine: Some("dropdb") })'

You can see it here for example: https://github.com/p2panda/aquadoggo/runs/7022667094?check_suite_focus=true The first test fails "correctly" and then all the others fail because of this lock.

I couldn't find a way to disconnect from databases automatically when something fails, except of manually catching every error and doing it manually .. maybe you have an idea?

Also, by testing a "real" database, we can't run the tests concurrently, we can assure this by adding -- --test-threads=1 to cargo test. We could rename the used database every time to something random, this would give us concurrency and also solve the locking problem of failing tests, but then you have like thousands of random databases left in your PostgreSQL? Not sure what to do here ..

I assume as soon as all tests are working again and we manually disconnect from the databases after every test it will go fine without any further changes.

Our SQL queries

Otherwise, we get what we want, which is feedback on how it works with other databases and unluckily there are a few things failing.

I've made some first fixes as commented in this PR, but there is more and I don't know if this is the place to fix that? I'd be fine doing it here.

Here is an example of a failing test:

FatalStorageError("error occurred while decoding column \"previous_operations\": unexpected null; try decoding as an `Option`")', aquadoggo/src/db/stores/document.rs:609:14

Where apparently previous_operations is a NULL column but in the deserialized OperationRow its not an Option. Funny, that SQLite didn't cry about this 😅

.. and wow, what was that? #170 (comment) 😰

adzialocha · 2022-06-24T11:33:13Z

aquadoggo/src/db/stores/test_utils.rs

@@ -248,7 +248,7 @@ impl TestDatabaseRunner {
            pool.close().await;

            // Panic here when test failed to propagate it further
-            assert!(result.is_ok());
+            result.unwrap();


I believe this will make #[should_panic] work again

Ah, DOH!!! Yeh, hopefully, why didn't I think of that...

I've tested it, it works again 👍

adzialocha · 2022-06-24T11:53:04Z

aquadoggo/migrations/20220617115933_create-tasks.sql

-    COALESCE(document_id, 0),
-    COALESCE(document_view_id, 0)
+    COALESCE(document_id, '0'),
+    COALESCE(document_view_id, '0')


This needs to be a string

adzialocha · 2022-06-24T11:54:00Z

aquadoggo/src/db/stores/entry.rs

-            database: UNIQUE constraint failed: entries.author, entries.log_id, entries.seq_num"
-        )
+            let result = db.store.insert_entry(duplicate_doggo_entry).await;
+            assert!(result.is_err());


Returned error strings are different depending on the used database, so we need to just check via is_err

sandreae

I'm really happy with this now.

Regarding the point I mentioned on the call earlier, I think seeing as we really are use rstest everywhere then there's no need to add another option now.

cafca · 2022-06-27T13:44:26Z

aquadoggo/src/db/stores/test_utils.rs

+}
+
+impl TestDatabaseRunner {
+    pub fn with_db_teardown<F: AsyncTestFn + Send + Sync + 'static>(&self, test: F) {


A docstring here could be friendly for when you encounter this function in some test. Or maybe just a reference to another place to learn more.

cafca · 2022-06-27T13:56:56Z

aquadoggo/src/db/stores/test_utils.rs

+            // be reached even when the test panicked
+            pool.close().await;
+
+            // Panic here when test failed to propagate it further


I am not sure I understand this docstring. Propagate the error further? Wouldn't this get triggered exactly when an error is propagated further and not when it is not? If this enables #[should_panic] that could also be nice to mention here for whoever edits this code next.

Not sure if I understand your question, but I'll try to explain it from another angle: Panicks in tasks are not propagated further ("unwinded"), they just crash the task. To allow the test runner to understand that something happend we have to take the task handle, unwrap it to inform the "parent" about its failure.

The function of this is to make the test fail when it should :-D

cafca

I had two wishes for docstrings and added some myself, otherwise it's all good!

* main: Update README.md UPSERT documents (#173) Run tests against PostgreSQL, fix compatibility (#170) End-to-end publishEntry tests (#167) Reschedule pending tasks during startup (#168)

* main: Update README.md UPSERT documents (#173) Run tests against PostgreSQL, fix compatibility (#170) End-to-end publishEntry tests (#167)

Make database url configurable for tests

6b42328

adzialocha linked an issue Jun 22, 2022 that may be closed by this pull request

CI: Tests should also run against PostgreSQL #31

Closed

adzialocha changed the title ~~Make database url configurable for tests~~ Run tests against PostgreSQL and MySQL databases Jun 22, 2022

adzialocha added 20 commits June 22, 2022 17:40

Add github action for postgres, probably this is the beginning of a l…

5a6ccd4

…onger try-and-error phase

Add --command

63b0df2

Add MySQL, try using services

2a0c0df

Enable logging

51908c5

Try different logging

21e41b6

Remove that fancy fluff

b5276a8

Fix comment

25567a1

Do not reset

bad9952

Who did that typo?

3dada54

Reset databases again

d21efa5

Correct database env var for postgres container

e22f704

What was that

a598bfe

Reduce parallelization

a0f86e1

Do not use BLOB

8c5199c

Make sure to disconnect when migration failed

0935387

Set mysql database again

1aad8f0

Correctly run tests serially

413cfbb

WIP

590dbd3

Fix column type

1ca7976

Clean up

05d2ad0

adzialocha commented Jun 23, 2022

View reviewed changes

aquadoggo/src/db/stores/document.rs Outdated Show resolved Hide resolved

adzialocha commented Jun 23, 2022

View reviewed changes

adzialocha added 2 commits June 24, 2022 11:36

Expose port properly

89efbd7

Remove MySQL support, fix async runtime of sqlx

ef08996

adzialocha force-pushed the postgres-ci branch from e6e3e45 to ef08996 Compare June 24, 2022 10:31

adzialocha changed the title ~~Run tests against PostgreSQL and MySQL databases~~ Run tests against PostgreSQL Jun 24, 2022

adzialocha changed the title ~~Run tests against PostgreSQL~~ Run tests against PostgreSQL, fix compatibility Jun 24, 2022

adzialocha added 6 commits June 24, 2022 12:34

Add entry to CHANGELOG.md

65130de

Give null values the same representation as index

c51434f

Make fields in other rows also optional

d2129dd

Last tests using old database utilities

7c1275c

Clean up

818139d

Remove unnecessary comments, just unwrap test result

6778ba4

adzialocha commented Jun 24, 2022

View reviewed changes

Make linters happy

0c0fbcc

adzialocha marked this pull request as ready for review June 24, 2022 11:51

adzialocha requested review from sandreae and cafca June 24, 2022 11:51

adzialocha commented Jun 24, 2022

View reviewed changes

Improve formatting of long case tests

e7fd225

sandreae approved these changes Jun 27, 2022

View reviewed changes

cafca reviewed Jun 27, 2022

View reviewed changes

Edit comments

5369d26

cafca approved these changes Jun 27, 2022

View reviewed changes

Add some more comments

cbad270

adzialocha merged commit ab8d186 into development Jun 27, 2022

adzialocha added a commit that referenced this pull request Jun 27, 2022

Merge branch 'main' into graphql-document-query-experimental

62ea4ba

* main: Update README.md UPSERT documents (#173) Run tests against PostgreSQL, fix compatibility (#170) End-to-end publishEntry tests (#167)

adzialocha added a commit that referenced this pull request Jun 27, 2022

Merge branch 'main' into schema-provider

b6ec8d6

* main: Update README.md UPSERT documents (#173) Run tests against PostgreSQL, fix compatibility (#170) End-to-end publishEntry tests (#167)

adzialocha deleted the postgres-ci branch June 29, 2022 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests against PostgreSQL, fix compatibility #170

Run tests against PostgreSQL, fix compatibility #170

adzialocha commented Jun 22, 2022 •

edited

codecov bot commented Jun 22, 2022 •

edited

adzialocha Jun 23, 2022

adzialocha Jun 24, 2022

adzialocha Jun 23, 2022 •

edited

adzialocha Jun 23, 2022

adzialocha Jun 23, 2022 •

edited

adzialocha Jun 24, 2022

adzialocha Jun 23, 2022

adzialocha commented Jun 23, 2022 •

edited

adzialocha Jun 24, 2022

sandreae Jun 24, 2022

adzialocha Jun 24, 2022

adzialocha Jun 24, 2022

adzialocha Jun 24, 2022

sandreae left a comment

cafca Jun 27, 2022 •

edited

cafca Jun 27, 2022

adzialocha Jun 27, 2022 •

edited

adzialocha Jun 27, 2022

cafca left a comment

Run tests against PostgreSQL, fix compatibility #170

Run tests against PostgreSQL, fix compatibility #170

Conversation

adzialocha commented Jun 22, 2022 • edited

📋 Checklist

codecov bot commented Jun 22, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adzialocha Jun 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adzialocha Jun 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adzialocha commented Jun 23, 2022 • edited

Small report

Problems of testing a database

Our SQL queries

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreae left a comment

Choose a reason for hiding this comment

cafca Jun 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adzialocha Jun 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cafca left a comment

Choose a reason for hiding this comment

adzialocha commented Jun 22, 2022 •

edited

codecov bot commented Jun 22, 2022 •

edited

adzialocha Jun 23, 2022 •

edited

adzialocha Jun 23, 2022 •

edited

adzialocha commented Jun 23, 2022 •

edited

cafca Jun 27, 2022 •

edited

adzialocha Jun 27, 2022 •

edited