test: Refactor utility functions verify_data and verify_schema to AssertionHelper class
#254
Conversation
amotl
force-pushed
the
testinfra-reuse
branch
from
daeb62d
to
c446d94
Compare
In PostgreSQL, all boils down to the `jsonb[]` type, but arrays are reflected as `sqlalchemy.dialects.postgresql.ARRAY` instead of `sqlalchemy.dialects.postgresql.JSONB`. In order to prepare for more advanced type mangling & validation, and to better support databases pretending to be compatible with PostgreSQL, the new test cases exercise arrays with different kinds of inner values, because, on other databases, ARRAYs may need to have uniform content. Along the lines, it adds a `verify_schema` utility function in the spirit of the `verify_data` function, refactored and generalized from the `test_anyof` test case.
Dispose the SQLAlchemy engine object after use within test utility functions.
Within `BasePostgresSDKTests`, new database connections via SQLAlchemy haven't been closed, and started filling up the connection pool, eventually saturating it.
Dispose the SQLAlchemy engine object after use within `PostgresConnector`.
| @pytest.fixture | ||
| @pytest.fixture(scope="session") | ||
| def postgres_target(postgres_config) -> TargetPostgres: | ||
| return TargetPostgres(config=postgres_config) |
There was a problem hiding this comment.
Hi. We have been enjoying a conversation about a potential resource leak.
@BuzzCutNorman said:
If I am following the test code correctly each test is creating an engine/connection pool and not closing or disposing of the engine. I think this is causing you to reach the limit of connections. My suggestion would be to dispose of the engine at the end of each test that generates an engine to see if that helps.
I just wanted to pick this up in order to outline that the problem has been solved differently here, by making the provided TargetPostgres object session-scoped.
I think doing it this way is the wrong choice, and hides any underlying issues. In that spirit, I think the fixes on behalf of GH-250 are more correct, as they are fixing details closer to the root cause by explicitly disposing SQLAlchemy engine objects.
There was a problem hiding this comment.
^^ This code has been removed again. The test fixtures are function-scoped, like before. As such, they reasonably verify that there are no resource leaks like outlined at GH-260.
By wrapping them into a container class `AssertionHelper`, it is easy to parameterize them, and to provide them to the test functions using a pytest fixture. This way, they are reusable from database adapter implementations which derive from PostgreSQL. The motivation for this is because the metadata column prefix `_sdc` needs to be adjusted for other database systems, as they reject such columns, being reserved for system purposes. In the specific case of CrateDB, it is enough to rename it like `__sdc`. Sad but true.
amotl
force-pushed
the
testinfra-reuse
branch
from
87b2437
to
860baf4
Compare
About
This patch is meant to make the test infrastructure more reusable for database adapters which derive from
target-postgres. GH-250 added averify_schemautility function, and this patch wraps up bothverify_dataand the new one into a container class, calledAssertionHelper.Note
This patch is stacked on top of GH-250, and as such, can't be reviewed well, because it includes the other changes. The specific commit of interest is daeb62d.