Skip to content
New issue

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

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

Already on GitHub? # to your account

Add simple test infrastructure for testing DB queries #1892

Merged
merged 6 commits into from
Feb 14, 2025

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jan 24, 2025

Especially with the recent review preferences changes, I think that the queries that we run in triagebot are now becoming too complex for them to be tested only in production, and we should start doing at least some basic testing.

There have been previous attempts at introducing more comprehensive tests (#1678, #1698). These were quite large in scope and got stuck discussing various things.

This PR attempts to do something much smaller. It doesn't add full end-to-end tests, but simply enables us to connect to an actual Postgres DB, setup some fixtures and then check that triagebot is doing what we expect it to. Thsi is essentially the same thing that I already do in new bors, with the only difference that there it is handled by sqlx, so it's one line of code :) But given previous discussions in the linked PRs, I wanted to avoid switching to sqlx here.

Note that I don't see any point in testing triagebot against SQLite. A lot of issues that we encounter in production are database and query issues. If we tested against SQLite, we would both have to implement all queries for it, which is additional work, and even then we would be testing something completely different than what gets run on production. Even if we get rid of the more esoteric things that we use now (Postgres arrays), there are still non-trivial differences between SQLite and Postgres (hell, even between different Postgres versions...), so I am quite sute that it wouldn't be worth it. Spinning up Postgres locally with Docker is trivial these days, but I made the tests opt-in anyway, so that it is not even required for running cargo test.

This PR contains a (currently commented out) test for the SQL query which should be hopefully fixed by #1891.

Best reviewed commit by commit. The first commit is the only one that could change production behavior, AFAIK, as the background job now shares the client pool with the main client logic. I think that should be fine, but if you want, I can get rid of that.

@apiraino
Copy link
Contributor

apiraino commented Jan 27, 2025

My understanding is that the problem is not how to test locally. As you point out it's easy to setup a local Postgres (I have that and test the DB part as rigorously as I can). A more impactful issue is not having visibility on the production DB, which leaves us blind when something goes wrong.

A simple testing infrastrucure is a welcome addition but it doesn't solve my problem. Ideally I'd need a way to peek into the production DB, right after an event I want to investigate (ex. it happened that a query returned the wrong result but only the first time it run)

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 27, 2025

Sure, that's a separate issue. If we had a chance to write tests for the new queries, it would be much easier to debug how they work :)

@apiraino
Copy link
Contributor

that's a separate issue.

Agreed, it's a separate issue - I just wanted to make it clearer that the opening comment is a bit orthogonal and not strictly related to having (or not) unit tests:

Especially with the recent review preferences changes, I think that the queries that we run in triagebot are now becoming too complex for them to be tested only in production, and we should start doing at least some basic testing.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 30, 2025

Well, I think that also the old behavior of triagebot was too complex for us to afford not having tests 😆 In general, we should just have tests :)

@jackh726
Copy link
Member

Sorry for the delay!

@jackh726 jackh726 merged commit 5cc4c25 into rust-lang:master Feb 14, 2025
3 checks passed
@Kobzol Kobzol deleted the db-tests branch February 14, 2025 21:45
@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 14, 2025

Absolutely no problem, thanks for taking a look ❤️ I will try to implement tests, starting with reviewer assignments, as a next step.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants