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(studio): Mutation & Node-API usage tests for Studio #1803

Merged
merged 9 commits into from
Jun 24, 2021

Conversation

sdnts
Copy link
Contributor

@sdnts sdnts commented Jun 14, 2021

No description provided.

@sdnts
Copy link
Contributor Author

sdnts commented Jun 14, 2021

I'm realizing there's no actual way to tell if Studio used the library or not. The only thing we can check is if the library was downloaded.

This iis because IIUC SDK needs the binary at least once to parse the schema. If the schema says it is supposed to use the library, it will (or it should at least) use it going forward. Same holds for Studio. So all this test does is make sure that libraries are downloaded when they should.

I learned this because in a previous commit, I was deleting binaries before the test if PRISMA_FORCE_NAPI was set, and then expecting that only libraries exist after the test. But that's not possible, because the SDK needs hthe bianry to parse the schema in the first place.

I was incorrect.

@janpio
Copy link
Contributor

janpio commented Jun 14, 2021

No, the CLI/SDK should already use the engine library when you install it with the env var in the first place. No query engine binary necessary at all.

@sdnts
Copy link
Contributor Author

sdnts commented Jun 15, 2021

@janpio Yeah but if I delete all binaries before the Node-API test starts, how would the SDK know to download the Node-API at all? It needs the binary at least once right?

(Look at my test.sh file for context)

Not relevant anymore.


parentId Int
parent User @relation(fields: [parentId], references: [id])
children User[] @relation("UserToUser")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was just causing problems while deleting etc. I have a test for self-referential models in Studio, so all good

@@ -1,13 +1,49 @@
#!/bin/sh

set -eu
set -e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed -u because we do not want to stop the script for undefined variables (in our case, PRISMA_FORCE_NAPI might be undefined)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use a test here that considers that - most of the other scripts that contain PRISMA_FORCE_NAPI do that.

@sdnts sdnts changed the title test(studio): Add a create test for Studio test(studio): Mutation & Node-API usage tests for Studio Jun 15, 2021
@sdnts sdnts marked this pull request as draft June 17, 2021 11:15
@sdnts sdnts self-assigned this Jun 24, 2021
echo '[2] QE binary exists when it should not'
exit 1
fi
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are some assertions that should check if anything extra was downloaded.

If you look on line 11, I delete binaries if PRISMA_FORCE_NAPI is set, and then check again here if they exist. If they do, it means someone downloaded them, and the test fails.

The same happens if PRISMA_FORCE_NAPI is not set, libraries are deleted before jest, and checked after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if all paths I check are sufficient, can obviously add other places we expect to find the binary / library.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Windows the paths should be different for the Node-API library, but I assume that is not being tested here so does not affect things.

@sdnts sdnts marked this pull request as ready for review June 24, 2021 14:30
@sdnts
Copy link
Contributor Author

sdnts commented Jun 24, 2021

@janpio This is ready for review! Merge whenever!

@janpio janpio merged commit 45a1d77 into dev Jun 24, 2021
@janpio janpio deleted the feat/studio-create-test branch June 24, 2021 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants