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

ci: run only external deps tests #96

Merged
merged 1 commit into from
Jan 16, 2025
Merged

ci: run only external deps tests #96

merged 1 commit into from
Jan 16, 2025

Conversation

seqre
Copy link
Member

@seqre seqre commented Jan 12, 2025

This is done to fix https://github.com/cot-rs/cot/actions/runs/12728982874/job/35480378199#step:8:441

Also, it runs only ignored tests to speed up the pipeline.

@seqre seqre added the ci Continuous Integration label Jan 12, 2025
@seqre seqre requested review from m4tx and Iipin January 12, 2025 10:58
@seqre seqre self-assigned this Jan 12, 2025
@m4tx
Copy link
Member

m4tx commented Jan 12, 2025

Why would this fix the issue? The tables/databases are not shared between tests, so I can't see why disabling parallelization would fix this. I was suspecting some data race or something similar when connecting to Postgres (removing the database is done from a separate connection, so I thought there might be something funky going on here).

@m4tx
Copy link
Member

m4tx commented Jan 12, 2025

As suspected, I just got the same error when testing locally with --run-ignored only --test-threads 1. Admittedly though, it took like hundreds of test suite re-runs in a while true loop.

It would be nice to either find out the real cause of the issue, or maybe just live in ignorance and add WITH (FORCE) to the DROP DATABASE?

--run-ignored only sounds like a good change, though.

Copy link

codecov bot commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
rust 87.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@seqre
Copy link
Member Author

seqre commented Jan 13, 2025

I get it now, I think we should merge this PR as is and create a task to investigate that issue separately. What do you think @m4tx?

@seqre seqre changed the title ci: remove parallelization of external deps tests ci: run only external deps tests Jan 13, 2025
@m4tx
Copy link
Member

m4tx commented Jan 15, 2025

I get it now, I think we should merge this PR as is and create a task to investigate that issue separately. What do you think @m4tx?

I think there are two sensible solutions for now:

  1. Leave the PR as is and merge it
  2. Add WITH (FORCE) to the DROP DATABASE statement (perhaps it's the way to go? I'm not sure if there's much value in investigating why this fails)

If you feel like 2 is not sensible or don't have time to do that, we can just merge this now.

@seqre
Copy link
Member Author

seqre commented Jan 16, 2025

I think it's worth playing around with it and trying to figure that out, if possible with minor effort. I've created a ticket to track it: https://github.com/orgs/cot-rs/projects/2/views/7?pane=issue&itemId=94050368 If no root cause is found, we can always default to that WITH (FORCE) as a last resort.

In the meantime, I'll merge this.

@seqre seqre merged commit d85ce8e into master Jan 16, 2025
25 checks passed
@seqre seqre deleted the ci-dep-test branch January 16, 2025 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants