Add schema versioning for database migrations

[pmachapman]

Fixes #567

---

This change is 

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.94%. Comparing base (a4e5a8e) to head (d428d92).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
+ Coverage   67.52%   67.94%   +0.41%     
==========================================
  Files         385      386       +1     
  Lines       21107    21205      +98     
  Branches     2748     2736      -12     
==========================================
+ Hits        14253    14408     +155     
+ Misses       5870     5812      -58     
- Partials      984      985       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Enkidu93]

So far so good! I forget - are you planning on making a more dedicated place for migration tests?

@Enkidu93 reviewed 11 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ddaspit).

[pmachapman]

I forget - are you planning on making a more dedicated place for migration tests?

I have added integration tests for SIL.DataAccess, with a sample test for a migration. Is this the sort of migration testing you were thinking of?

@pmachapman made 1 comment.
Reviewable status: 11 of 29 files reviewed, all discussions resolved (waiting on ddaspit).

[Enkidu93]

Yes, that's great! Do you think the content from MongoMigrations and the test MongoMigration_TargetQuoteConvention in TranslationEngineTests should be relocated/refactored at all to fit with this new arrangement?

@Enkidu93 reviewed 18 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ddaspit).

[ddaspit]

@ddaspit reviewed 29 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).

---

src/DataAccess/src/SIL.DataAccess/IMongoDataAccessConfiguratorExtensions.cs line 9 at r2 (raw file):

        string collectionName,
        Action<BsonClassMap<T>>? mapSetup = null,
        Func<IMongoCollection<T>, Task>[]? init = null

Nit: IReadOnlyList would make this slightly more flexible.

---

src/DataAccess/test/SIL.DataAccess.IntegrationTests/ServalSharedTests.cs line 5 at r2 (raw file):

[TestFixture]
[Category("Integration")]
public class ServalSharedTests

The fact that these tests have a dependency on Serval projects makes me feel like they should be in the Serval integration tests. Alternatively, you could create a test DB setup, since these tests are not testing Serval but the data access migrations.

[ddaspit]

@ddaspit made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).

---

src/DataAccess/test/SIL.DataAccess.IntegrationTests/ServalSharedTests.cs line 5 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The fact that these tests have a dependency on Serval projects makes me feel like they should be in the Serval integration tests. Alternatively, you could create a test DB setup, since these tests are not testing Serval but the data access migrations.

Actually, I think I misunderstood the purpose of these tests. These actually are testing the Serval migrations, right? I would prefer that these are included in Serval integration tests.

[pmachapman]

Do you think the content from MongoMigrations and the test MongoMigration_TargetQuoteConvention in TranslationEngineTests should be relocated/refactored at all to fit with this new arrangement?

I don't think it needs to be. The existing test does the job well, and uses the model. If it the old property gets dropped from the model, then it would make sense to move.

@pmachapman made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit).

---

src/DataAccess/src/SIL.DataAccess/IMongoDataAccessConfiguratorExtensions.cs line 9 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Nit: IReadOnlyList would make this slightly more flexible.

Done. Thanks!

---

src/DataAccess/test/SIL.DataAccess.IntegrationTests/ServalSharedTests.cs line 5 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Actually, I think I misunderstood the purpose of these tests. These actually are testing the Serval migrations, right? I would prefer that these are included in Serval integration tests.

@ddaspit Do you mean the API integration tests? These didn't feel like that quite fit there, as the tests in that project communicate with the API directly.

Also, one of the test classes is for Serval.Machine (which did not previously have any tests outside of e2e testing the migrations), while the other class is for Serval.

I wonder if I should create two projects - Serval.Machine.IntegrationTests, and Serval.IntegrationTests? I created the SIL.DataAccess.Integration tests project as I couldn't figure out just where to place these tests.

If you still want these in Serval.ApiServer.IntegrationTests, not a problem, I just thought I'd explain my reasoning before doing that.

[ddaspit]

@ddaspit reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).

---

src/DataAccess/test/SIL.DataAccess.IntegrationTests/ServalSharedTests.cs line 5 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

@ddaspit Do you mean the API integration tests? These didn't feel like that quite fit there, as the tests in that project communicate with the API directly.

Also, one of the test classes is for Serval.Machine (which did not previously have any tests outside of e2e testing the migrations), while the other class is for Serval.

I wonder if I should create two projects - Serval.Machine.IntegrationTests, and Serval.IntegrationTests? I created the SIL.DataAccess.Integration tests project as I couldn't figure out just where to place these tests.

If you still want these in Serval.ApiServer.IntegrationTests, not a problem, I just thought I'd explain my reasoning before doing that.

Yeah, I get what you mean. They don't really fit in the API integration tests, since they aren't testing the REST API. The Machine tests certainly don't belong in that project. You should definitely create a Serval.Machine.Shared.IntegrationTests and move the Machine tests there. The right thing to do would be to create a new Serval.IntegrationTests project. If you don't feel like doing that, I wouldn't have a problem if you move them to the Serval.ApiServer.IntegrationTests project, since it should have all of the required dependencies. If you do create a new Serval.IntegrationTests project, then go ahead and move the existing migration test there.

[pmachapman]

@pmachapman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit).

---

src/DataAccess/test/SIL.DataAccess.IntegrationTests/ServalSharedTests.cs line 5 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Yeah, I get what you mean. They don't really fit in the API integration tests, since they aren't testing the REST API. The Machine tests certainly don't belong in that project. You should definitely create a Serval.Machine.Shared.IntegrationTests and move the Machine tests there. The right thing to do would be to create a new Serval.IntegrationTests project. If you don't feel like doing that, I wouldn't have a problem if you move them to the Serval.ApiServer.IntegrationTests project, since it should have all of the required dependencies. If you do create a new Serval.IntegrationTests project, then go ahead and move the existing migration test there.

Done.

[ddaspit]

@ddaspit partially reviewed 19 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

[Enkidu93]

@Enkidu93 partially reviewed 20 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pmachapman).