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

Make SyncFixture users use seperate directories and clean them up #1311

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

myieye
Copy link
Contributor

@myieye myieye commented Dec 10, 2024

It seemed to me that the Directory.Delete calls in InitializeAsync were trying to delete files being used by other tests (and causing failures for me). So, I've adapted the Fixture to use a different directory per SyncFixture. That means we don't know how to clean up old test projects on start, so I'm cleaning them up at the end.

It took me a while to figure out how to close pooled open connections. Largely, because there are 3 different SqliteConnection classes to choose from and they all have ClearAllPools(), but I overlooked the 3rd one and that's the one I actually needed 😆.

It also not clear to me how we'd only clear the connections to just a single db if we wanted to delete it in e.g. prod.

For a while I thought that the db file was still in use after the tests, because CrdtProjectsService.CreateProject() has var db = serviceScope.ServiceProvider.GetRequiredService<LcmCrdtDbContext>(); and doesn't dispose the db afterwards. But disposing it didn't help. And I believe that DbContext will get disposed at the end of the current scope?

@myieye myieye marked this pull request as draft December 11, 2024 07:45
@myieye myieye force-pushed the fixup-sync-fixture branch from 6e8d198 to d2804e0 Compare December 11, 2024 12:28
@myieye myieye marked this pull request as ready for review December 11, 2024 13:32
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

I've found the most reliable way to cleanup a db is to use DbContext.Database.EnsureDeleted(). That said I would prefer if we could maintain the current functionality where it cleans up at the start. I think we can get the best of both worlds though by using a singleton to track if we've already done our cleanup. So in InitializeAsync we try to delete the projects from the previous test run, right now we do that every time a SyncFixture is created (causing the issue you are trying to fix), what if we only did that cleanup once using a static bool to track that? We would need to us a lock to avoid race conditions.

@myieye myieye force-pushed the fixup-sync-fixture branch from d2804e0 to f6715c1 Compare December 13, 2024 14:20
@myieye myieye force-pushed the fixup-sync-fixture branch from f6715c1 to 401ffcf Compare December 13, 2024 14:21
@myieye
Copy link
Contributor Author

myieye commented Dec 13, 2024

I've changed it to only clean up on start again

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

Looks good to me, I might use a HashSet instead of a dictionary considering we never read the value, but it doesn't matter here.

@myieye myieye merged commit 67896c5 into develop Dec 18, 2024
8 checks passed
@myieye myieye deleted the fixup-sync-fixture branch December 18, 2024 10:14
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