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

chore: test with postgres #301

Merged
merged 2 commits into from
Mar 10, 2023
Merged

Conversation

dopry
Copy link
Collaborator

@dopry dopry commented Jan 16, 2023

While working on search I discovered that sqllite couldn't support relevance scoring to validate natural ordering and frankly most production sites don't use sqlite. This adds postgres to the test matrix.

@dopry dopry force-pushed the feat/postgres-testing branch from 7bb45d1 to 52e57ca Compare January 16, 2023 23:23
@dopry dopry mentioned this pull request Jan 17, 2023
@dopry
Copy link
Collaborator Author

dopry commented Jan 17, 2023

pre-commit.ci autofix

@dopry dopry force-pushed the feat/postgres-testing branch 6 times, most recently from 494d919 to 9514d99 Compare January 17, 2023 18:06
README.md Outdated Show resolved Hide resolved
example/example/tests/test_grapple.py Outdated Show resolved Hide resolved
example/example/tests/test_grapple.py Outdated Show resolved Hide resolved
@dopry dopry force-pushed the feat/postgres-testing branch 2 times, most recently from b3600d0 to ce54e4f Compare January 17, 2023 18:22
@dopry
Copy link
Collaborator Author

dopry commented Jan 17, 2023

@zerolab not sure what to do about precommit/prettier complaining about the migrations. There were a few linter changes that just popped up that I'm pretty sure aren't related to my code.

bugbear start spitting out B028's where it shouldn't be afaict, we actually want those lines to have the explicit quotes and we want the id as is and we don't actually want __repr__ in those cases.

@zerolab
Copy link
Member

zerolab commented Jan 17, 2023

I will have a look at the linting issue.

@zerolab
Copy link
Member

zerolab commented Jan 17, 2023

#303 for linting. I am happy with fully ignoring B028

@dopry dopry force-pushed the feat/postgres-testing branch from ce54e4f to 355c4ef Compare January 17, 2023 19:58
@dopry
Copy link
Collaborator Author

dopry commented Jan 17, 2023

alright. Rebased on main with #303 to hopefully resolve the linting issues. I'll need to hang up my hat here for the time being pending resolution to wagtail/wagtail#9788 vs updating setUpTestData back to setUp.

I would really love it if we could get #299 merged in the interim.

@dopry dopry force-pushed the feat/postgres-testing branch 6 times, most recently from 012690b to 65bd8a2 Compare March 8, 2023 17:37
@dopry dopry marked this pull request as draft March 8, 2023 17:45
@dopry dopry force-pushed the feat/postgres-testing branch 2 times, most recently from 32098f6 to f0297e9 Compare March 8, 2023 18:12
@dopry dopry marked this pull request as ready for review March 8, 2023 18:51
@dopry
Copy link
Collaborator Author

dopry commented Mar 8, 2023

@zerolab hey I got it all running, but black says everything is good locally for me, so I don't know what exactly pre-commit is complaining about.

@zerolab
Copy link
Member

zerolab commented Mar 8, 2023

pre-commit.ci autofix

let's try to see if this does anything

@dopry
Copy link
Collaborator Author

dopry commented Mar 9, 2023

@zerolab danke. you've earned the bot whisperer badge.

@zerolab
Copy link
Member

zerolab commented Mar 9, 2023

🤖
on a serious note: I should have some time tomorrow to do a proper review

@dopry dopry force-pushed the feat/postgres-testing branch from f67314b to d00eb49 Compare March 10, 2023 15:34
Copy link
Member

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Got a couple of questions, otherwise LGTM

@@ -915,7 +916,6 @@ def test_blog_page_tags(self):
tags = executed["data"]["page"]["tags"]
self.assertEqual(len(tags), 3)
for idx, tag in enumerate(tags, start=1):
self.assertEqual(int(tag["id"]), idx)
Copy link
Member

Choose a reason for hiding this comment

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

question: why remove it? (I suspect I know the answer, but best not to assume)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no guarantee that tags are ordered by ID or that a given database's auto incremented will start the ids at 0 and advance by 1. This is simply a frail assertion that doesn't prove anything about the validity of the data returned by the database.

example/home/test/test_general.py Outdated Show resolved Hide resolved
example/home/test/test_general.py Show resolved Hide resolved
grapple/utils.py Show resolved Hide resolved
@dopry dopry force-pushed the feat/postgres-testing branch from d00eb49 to 5e88da8 Compare March 10, 2023 16:08
@zerolab zerolab merged commit 7c32128 into torchbox:main Mar 10, 2023
@dopry dopry deleted the feat/postgres-testing branch March 10, 2023 16:46
engAmirEng pushed a commit to engAmirEng/wagtail-grapple that referenced this pull request May 2, 2023
* chore: test with postgres

* fix: images tests on postgres
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.

2 participants