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

[Pg] Add tests for Postgres ->> and -> operators #1560

Closed
wants to merge 2 commits into from

Conversation

serdnam
Copy link

@serdnam serdnam commented Nov 26, 2023

This PR adds tests only, related to issue #1511. The tests assert whether the -> and ->> operators work after inserting json or jsonb values into Postgres.

The included tests include a failing test for the postgres.js integration, it seems it does not properly handle stringified objects as input to JSON/JSONB columns, I have prepared a demo of this behavior here: https://github.com/serdnam/postgres-js-json-behavior.

I tried my hand at solving it, but I do not know the codebase well enough to know how or where to make a change to fix this (or if the change has to be done on drizzle's side at all, it seems there is a related postgres.js issue porsager/postgres#392)

Added tests for the Postgres ->> and -> operators, including a known failing test for the Postgres.js integration. The reason for this test failing is due to the way Postgres.js handles stringified JSON input.
@Angelelz
Copy link
Collaborator

If there is a failing test I'm not sure this can be merged. I can work on the underlying issue and if you are ok with it, I can include your code in my PR. I don't think I'll have time to open the PR and have it reviewed before the next minor release but I'll give it a try

@serdnam
Copy link
Author

serdnam commented Nov 28, 2023

The test does not fail the whole test suite, since it is declared using the failing function from ava (test.serial.failing). But if that is still an issue, I have no issue with you including my code in your PR.

@Angelelz
Copy link
Collaborator

Hey @serdnam, since the fix should be merged soon, I guess can you delete the failing part? That way when this is merged we wont have issues.

@serdnam
Copy link
Author

serdnam commented Dec 13, 2023

Sure, I'll delete it.

@Angelelz
Copy link
Collaborator

Angelelz commented Dec 13, 2023

My apologies, I should have said just delete the word failing, so it's a regular test

Edit: what I mean is, the test should stay

@serdnam
Copy link
Author

serdnam commented Dec 13, 2023

No worries! I restored the test without the failing option.

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.

3 participants