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(qe): driver adapters: parameterize test kit to test any driver adapter that we support #4265

Merged
merged 15 commits into from
Sep 21, 2023

Conversation

miguelff
Copy link
Contributor

@miguelff miguelff commented Sep 20, 2023

This PR changes the executor for driver adapters to instantiate a different driver adapter based on environment configuration.

From the chunk of documentation added to the connector kit README:

Running tests through driver adapters

The query engine is able to delegate query execution to javascript through driver adapters.
This means that instead of drivers being implemented in Rust, it's a layer of adapters over NodeJs drivers the code that actually communicates with the databases.

To run tests through a driver adapters, you should also configure the following environment variables:

  • NODE_TEST_EXECUTOR: tells the query engine test kit to use an external process to run the queries, this is a node process running
    a program that will read the queries to run from STDIN, and return responses to STDOUT. The connector kit follows a protocol over JSON RPC for this communication.
  • DRIVER_ADAPTER: tells the test executor to use a particular driver adapter. Set to neon, planetscale or any other supported adapter.
  • DRIVER_ADAPTER_URL_OVERRIDE: it overrides the schema URL for the database to use one understood by the driver adapter (ex. neon, planetscale)

Example:

export NODE_TEST_EXECUTOR="$WORKSPACE_ROOT/query-engine/driver-adapters/js/connector-test-kit-executor/script/start_node.sh"
export DRIVER_ADAPTER=neon
export DRIVER_ADAPTER_URL_OVERRIDE ="postgres://USER:PASSWORD@DATABASExxxx"

Closes https://github.com/prisma/team-orm/issues/364

@miguelff miguelff force-pushed the miguelff/parameterized-test-kit branch from cbb5c94 to 5668de8 Compare September 20, 2023 14:28
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 20, 2023

CodSpeed Performance Report

Merging #4265 will degrade performances by 5.38%

Comparing miguelff/parameterized-test-kit (0981e15) with main (a8d82f7)

Summary

❌ 1 regressions
✅ 10 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main miguelff/parameterized-test-kit Change
large_read 7.3 ms 7.7 ms -5.38%

@janpio janpio added the topic: driver adapters formerly phase 1 label Sep 20, 2023
@miguelff miguelff marked this pull request as ready for review September 20, 2023 16:54
@miguelff miguelff requested a review from a team as a code owner September 20, 2023 16:54
Sometimes, it might even make sense. You might want an error message to say:

"This should have been a valid URL but it was undefined"

rather than:

"This should have been a valid URL but it was "

but it is better if it was:

rather than:

"This should have been a valid URL but it was ''"
@miguelff miguelff requested a review from aqrln September 21, 2023 09:04
@miguelff
Copy link
Contributor Author

Feedback addressed. Thank you so much for the review @aqrln!

@miguelff
Copy link
Contributor Author

miguelff commented Sep 21, 2023

I piggybacked 2c6c5d4 to fix an inconsistency in the naming of the entity that does the processing of requests, to align with the notion of an external test executor.

}

async function neonAdapter(_: string): Promise<DriverAdapter> {
const connectionString = process.env.DRIVER_ADAPTER_URL_OVERRIDE ?? ''
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking in any way and can be addressed later if we decide to change it, but I wonder if it would actually be better to have separate environment variables (e.g. DRIVER_ADAPTER_NEON_URL etc) for each adapter, if the current DRIVER_ADAPTER_URL_OVERRIDE is not being read in any centralised way anyway and each adapter constructor function is responsible for reading it on its own.

Some reasoning behind it:

  • It's not always just one variable per adapter, some adapters have none (pg), some have two or three (Turso).
  • It would be possible to set all of them in .envrc.local once and not comment/uncomment stuff or source separate files to switch between adapters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I designed this I opted for the opposite reasoning. Which is, whatever you need is by an adapter, if any, is in that variable. For instance, if it has 3 urls, it can be coalesced into a single env var, joined by a separator.

The reason for that choice is that you don't need to remember which is the variables per connector for tests. This behavior is symmetric with how we test other drivers: ex. we use a single URL variable for all connectors, and then we use a variable connector.

I see smoke tests take the opposite approach, the one you mentioned. I think we should aim at consistency, and have something akin to what we have when testing a rust connector, which is a set of variables being set accordingly, when we issue a make dev-* command. Rather than having all the options in different variables in the .envrc.

I'm open to discussing this further async.

@miguelff miguelff force-pushed the miguelff/parameterized-test-kit branch from b51ee23 to 0981e15 Compare September 21, 2023 10:36
@aqrln aqrln added this to the 5.4.0 milestone Sep 21, 2023
@aqrln aqrln merged commit 0872812 into main Sep 21, 2023
51 of 55 checks passed
@aqrln aqrln deleted the miguelff/parameterized-test-kit branch September 21, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: driver adapters formerly phase 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants