Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Add integration tests for Indexer #627

Merged
merged 16 commits into from
Mar 29, 2024
Merged

Conversation

morgsmccauley
Copy link
Collaborator

@morgsmccauley morgsmccauley commented Mar 29, 2024

This PR adds a very basic integration test for Indexer. It uses testcontainers to stand up both postgres and hasura so that Indexer can talk to real components rather than mocks. The test uses Indexer directly, which means S3/Redis are still somewhat mocked/ignored. We can add those in later if need be.

This is essentially just the scaffolding for integration testing which can be expanded over time. The suite includes only 1 very basic test, which if successful should provide a fair amount of confidence that things are working as expected. The flow includes: provisioning, writing data to Postgres, and then asserting its existence via GraphQL. All errors bubble up from Indexer so this test should catch most problems.

This PR points to #625, as so I could test the pg_cron flow via this integration test :)

@morgsmccauley morgsmccauley changed the base branch from main to feat/schedule-log-partition-jobs March 29, 2024 00:53
@morgsmccauley morgsmccauley changed the title test/runner integration test: Add integration tests for Indexer Mar 29, 2024
@morgsmccauley morgsmccauley linked an issue Mar 29, 2024 that may be closed by this pull request
@@ -0,0 +1,9 @@
FROM hasura/graphql-engine:latest.cli-migrations-v3
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically applies locally stored metadata/migrations (i.e. logs/state tables, append role etc.) prior to standing up graphql server - probably only useful in tests right now

@@ -0,0 +1,11 @@
FROM postgres:14
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postgres image which enables pg_cron

@@ -0,0 +1,5 @@
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate tsconfig to use when building, the default tsconfig.json should only be used locally now since it includes tests files etc.

@@ -96,9 +108,9 @@ export default class Provisioner {
const userCronPgClient = new this.PgClient({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the connection parameters returned from hasura will include the host/port used within the docker network, therefore we need an override

database: connectionParams.database,
...poolConfig,
});

this.pgPool.on('error', onError);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pgPool will throw an error if the connection is closed from the server side - which will be the case during integration tests as the container will shut down first. Catching it here to prevent runtime errors.

@@ -49,6 +59,7 @@ export default class Indexer {
deps?: Partial<Dependencies>,
databaseConnectionParameters = undefined,
dmlHandler = undefined,
private readonly config: Config = defaultConfig,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to parameterise these so they are configurable from test

export default class HasuraClient {
static DEFAULT_DATABASE = 'default';
static DEFAULT_SCHEMA = 'public';

private readonly deps: Dependencies;

constructor (deps?: Partial<Dependencies>) {
constructor (deps?: Partial<Dependencies>, private readonly config: Config = defaultConfig) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameterising so they are configurable from tests

@morgsmccauley morgsmccauley marked this pull request as ready for review March 29, 2024 01:11
@morgsmccauley morgsmccauley requested a review from a team as a code owner March 29, 2024 01:11
Copy link
Collaborator

@gabehamilton gabehamilton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

Base automatically changed from feat/schedule-log-partition-jobs to main March 29, 2024 20:04
Copy link
Collaborator

@darunrs darunrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat work! This will be fun to build on top of to get a good quite of integ tests going.

Just some small comments.


RUN echo "shared_preload_libraries = 'pg_cron'" >> /usr/share/postgresql/postgresql.conf.sample

RUN echo "CREATE EXTENSION pg_cron;" > /docker-entrypoint-initdb.d/init-pg-cron.sql
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably works. Though, I actually have more SQL that's necessary to set up pgBouncer. I created an sql file in queryApi which is copied into the docker entry point inside the docker compose. Since you're adding a dockerfile for postgres now, maybe we can merge the two approaches together?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can create a top level postgres/ directory which houses both this Dockerfile and the init script. I'll look at that in a follow up :)

@@ -10,17 +10,43 @@ interface SqlOptions {
source?: string
}

interface DatabaseConnectionParameters {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually export this interface in Indexer. You can import it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again related to #625, I'll fix in a follow up PR


await userCronPgClient.query(
this.pgFormat(
"SELECT cron.schedule_in_database('%1$I_logs_create_partition', '0 1 * * *', $$SELECT fn_create_partition('%1$I.__logs', CURRENT_DATE, '1 day', '2 day')$$, %2$L);",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can convert these into constants and label them something like CreatePartitionCronSql or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes are from #625 which this PR originally pointed to but has since been merged 😄

await indexer.runFunctions(
Block.fromStreamerMessage(block1 as any as StreamerMessage),
{
'morgs.near/test': {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works fine for now. How are you thinking of organizing the integ tests once we have more test cases and test code? Maybe a new folder called integ/ and label the code according to the test? Same for sample block data. Maybe we can also leverage the block-server to download actual blocks too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't put a lot of though in to that because I'm not entirely sure how these will evolve over time - we can tackle that when needed

@morgsmccauley morgsmccauley force-pushed the test/runner-integration branch from 7332228 to 7a4862a Compare March 29, 2024 20:21
@morgsmccauley morgsmccauley merged commit b7d43e1 into main Mar 29, 2024
3 checks passed
@morgsmccauley morgsmccauley deleted the test/runner-integration branch March 29, 2024 20:36
This was referenced Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Runner integration testing
3 participants