Containerized tests fixes

[Ste1io]

This PR is aimed to get the testing environment to a working state with containerized Docker images. It also includes some spring cleaning in the placement of some tests; however, I've refrained from any potential breaking changes or feature changes (such as fixing support for newer dbms versions not already specifically referenced or supported). I'm also aware that v7 branch already has a significant amount of improvements and changes to the test suites, so I've opted for a few TODO comments vs full-on fixes for most changes (virtual base class methods, updated dbms image versions, etc).

A few remaining tasks I'd like to fix in this PR still; if not, I'll convert them to issues and look into them later; see the commit messages in this PR for more info regarding the fixes.

• [x] setup docker env and update docker-compose.yml
  • requires Docker Desktop
• [x] fix Firebird tests failing with connection error
• [x] fix MySQL stored proc tests failing with error: "Character set 'utf8mb3' is not supported by .Net Framework"
• [x] fix MariaDB tests failing with errors "Object cannot be cast from DBNull to other types", "Character set 'utf8mb3' is not supported by .Net Framework", and "The given key was not present in the dictionary"
• [x] setup env and verify MsAccess tests
  • requires MS Access Database Engine 2010 Redistributable
• [x] remove MssqlCe tests - past EOL and dropping support in v7
• [x] fix MSAccess paging tests that shouldn't be run for that provider (provider doesn't provide paging, so tests aren't applicable)
• [x] fix MSAccess tests failing from syntax errors
• [x] fix SQLite tests experiencing db locking issues for streaming query tests causing timeouts and test failures.
• [x] documentation-style tests concurrency issues fixed when run in parallel with main test suite
• [x] split shared container images into separate images to isolate the test data from other tests running in parallel (currently causes roughly half of the tests to fail)
• [x] make all base class test functions virtual
• [x] fix async void tests to return Task to ensure xunit and assertions can propertly catch exceptions and return task completion status
• [x] fix shouldly assertions in async method not using async overload (such as Should.ThrowAsync)
• [x] update wiki pages detailing development/contributing, which still contains outdated instructions based on defunct tooling and VS2015 [mostly finished].
• [x] finish moving bugfix repro tests out of derived dbms-specific test classes into base test class

Final tasks related to tests, but to be handled in separate PRs:

• [x] Create new issue for: add integration test boilerplate classes for oracle; address #691 on that PR
• [x] Create new issue for: refactor test class inheritance to eliminate duplicate test code between db systems with multiple drivers, and differentiate driver-specific test cases from dbms-specific cases

docker-compose.yml has been updated to version 2. And to version 3. I think that's the latest???...lol

Docker image switched to Firebird 2.5 SuperClassic instead of "latest" to sidestep a plugin mismatch issue with the latest version (v4.0). 2.5 has been EOL for ages, so we should look at 3.0 and 4.0 and add an additional image as a followup.

Fixed a failing test for Firebird DB; the "NULL" keyword is not accepted when adding a new column in an ALTER TABLE statement, causing a syntax error. Noticed that many of the BaseQueryTests class methods are not marked as virtual, including the offending one here; for consistency, I'd like to see them all virtual, but opted to make that a todo for the moment. I've added this to my list 🤦🏻‍♂️ since half of the MsAccessQueryTests fail due to not having paging support, and need overridden for that provider...may as well just do them all at once and be done with it ig.

Fixed MySQL stored procedure tests that were all failing with "System.NotSupportedException : Character set 'utf8mb3' is not supported by .Net Framework." After a solid hour of searching online to no avail (even ai was no help), I finally tried a "SET NAMES" statement at the top of the sql build script after recalling seeing it in generated db dumps. That fixed it.

Fixed MariaDB tests that were failing with an odd (and rather random) sprinkling of the following two errors:

System.InvalidCastException : Object cannot be cast from DBNull to other types.

Stack Trace:  IConvertible.ToInt32(IFormatProvider provider) Driver.LoadCharacterSets(MySqlConnection connection) Driver.Configure(MySqlConnection connection)

System.NotSupportedException : Character set 'utf8mb3' is not supported by .Net Framework. Stack Trace: 

CharSetMap.GetCharacterSet(DBVersion version, String charSetName) MySqlField.SetFieldEncoding() NativeDriver.GetColumnData(MySqlField field)

This required specifying mariadb:10.9 for the image, as mentioned in this SO thread, as 10.10 changed the id column type to allow NULL. Doesn't exactly sit well with me, but I supposed this is somewhat the end user's responsibility in a sense. A more robust solution, IMO, would be to switch to the MySqlConnector as the underlying db provider for MariaDB, but that change would need to happen in it's own dedicated PR (and probably qualifies for at least a minor version bump).

Note that after downgrading the image version for MariaDB, including the same "SET NAMES" statement at the top of the build script as MySQL is still necessary to fix the "utf8mb3" charset error (both of them caused by the default encoding changes made by these dbms).

Fixed data integrity bug caused by using the same db image/port for all 4 mssql/mssqlmsdata connections; simultaneous connections running in parallel were causing data to be tainted, and creating countless errors when one test would attempt to cleanup while another was executing.

Documentation tests are now configured to run synchronously; it wasn't worth creating another image just for them, and given how small that group of tests is, whatever impact on the test execution timings is nominal if existent at all.

Fixed sqlite locking issues by running the sqlite in multithread mode vs the default (serialized). This isn't necessarily the safest design choice obviously, but in the sandboxed environment of the integration tests, concurrency is already being handled by the configured test runner, so there's no need for the additional database-level locking and serialization essentially becomes just a dead weight.

Also included some misc code cleanup, including typo corrections and script consistency improvements as I stumbled across them.

closes #676

[Ste1io]

The two failing doc tests are confirmed to pass with the changes introduced in #687 (not present in this branch).

[Ste1io]

Been having some difficulty wrangling the sqlite tests... When run as a batch with other tests, they behave nearly all the time, since there's a slight delay between each test incurred from running all the other parallel tests. If run without other test classes (say, batch testing the query tests), it's pretty much guaranteed to hit a deadlock after half a dozen or so. Haven't had any luck running it as an in-memory file either. As they are now, passing all tests is at the sole mercy of a race condition.

Feels hacky, but I may need to just have to add manual locking to that class to give the db some breathing room between tests. As it stands, sqlite is just too fragile to be relied on (even for tests that aren't returning a query stream), and once a connection is locked, you're pretty much screwed without restarting the running process. Unacceptable especially if expected to run in an automated workflow.

Any suggestions or ideas?

[Curlack]

Can we utilize something similar to what the guys discussed for PHP and Drupal?

https://www.drupal.org/project/drupal/issues/1120020

They mention starting the connection in IMMEDIATE mode and avoiding startTransaction exceptions in a try catch.

On Thu, 21 Sep 2023, 7:55 am Stelio Kontos @.***> wrote:

Been having some difficulty wrangling the sqlite tests... When run as a batch with other tests, they behave nearly all the time, since there's a slight delay between each test incurred from running all the other parallel tests. If run without other test classes (say, batch testing the query tests), it's pretty much guaranteed to hit a deadlock after half a dozen or so. Haven't had any luck running it as an in-memory file either. As they are now, passing all tests is at the sole mercy of a race condition.

Feels hacky, but I may need to just have to add manual locking to that class to give the db some breathing room between tests. As it stands, sqlite is just too fragile to be relied on (even for tests that aren't returning a query stream), and once a connection is locked, you're pretty much screwed without restarting the running process. Unacceptable especially if expected to run in an automated workflow.

Any suggestions or ideas?

— Reply to this email directly, view it on GitHub https://github.com/CollaboratingPlatypus/PetaPoco/pull/696#issuecomment-1728901370, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACI4HKCQZOPFU6CG3YENDDX3PJG5ANCNFSM6AAAAAA47TURS4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[Ste1io]

starting the connection in IMMEDIATE mode and avoiding startTransaction exceptions in a try catch.

Good point, worth a shot in my book. I saw a similar mention to that in a thread on the efcore repo earlier also. I'll try that tomorrow and see what kind of improvements come out of it.

Appreciate the suggestion.

[asherber]

Or just configure the SQLite tests not to run in parallel?

[asherber]

Oh, hmm, looks like the database-specific integration tests are already configured to run serially for each database. Is it these tests (in the Databases/Sqlite folder) the ones that are failing?

[Ste1io]

Or just configure the SQLite tests not to run in parallel?

They aren't running parallel (default behavior of XUnit for all tests from the same class). And running them separate from other tests actually increases the chance of them passing since there's more concurrency involved in the process as a whole.

All integration tests as a single run, usually works without incident, but not every time:

Testing each Sqlite class in batches, guaranteed lock hit after the first few tests:

[pleb]

Sqlite tests have been dodgy for a while now. I've updated them a few times to work with different sqlite providers. I might get time to have a little look today

[Ste1io]

Changing it to run in multi-thread mode instead of serialized mode (default) may actually do the trick...see:

https://sqlite.org/threadsafe.html and https://sqlite.org/compile.html#threadsafe

[Ste1io]

Fixed it. Will commit momentarily.

[asherber]

(I was thinking about across test classes, which xunit normally runs in parallel. I didn't realize until I went back to look at the code that the various test classes for each db are defined in a collection, which makes things serial across those classes.)

[Curlack]

Nicely done!

On Fri, 22 Sep 2023, 1:54 am Stelio Kontos @.***> wrote:

Fixed it. Will commit momentarily. [image: 1695340423-PetaPoco_-_Microsoft_Visual_Studio] https://user-images.githubusercontent.com/37424493/269783170-e68a6a45-b3ea-499b-bb3d-b87f40a36440.png

— Reply to this email directly, view it on GitHub https://github.com/CollaboratingPlatypus/PetaPoco/pull/696#issuecomment-1730506829, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACI4HMU3W4GVPL2MJLD26TX3THR5ANCNFSM6AAAAAA47TURS4 . You are receiving this because you commented.Message ID: @.***>

[Ste1io]

(I was thinking about across test classes, which xunit normally runs in parallel. I didn't realize until I went back to look at the code that the various test classes for each db are defined in a collection, which makes things serial across those classes.)

Got it, your previous comment does make sense in light of that. Given what you said about how xunit handles collections, would my additional collection definition class I made for the doc tests even be necessary? Changing the collection for those four files from "mssql" to "documentation", as I did in https://github.com/CollaboratingPlatypus/PetaPoco/pull/696/commits/54d05327fbf6f814a70d1f9977fb8d2ec1a5c30e, might be sufficient without the xunit attribute?

[asherber]

Xunit by default does not run tests within a collection in parallel. By default, again, each class is its own collection. Adding the [Collection] attribute to several classes makes them all part of the same collection, so none of the tests in those classes will be run in parallel with each other.

However, Xunit will run the tests of one collection in parallel with the tests in another collection; the collection definition class you added basically tells Xunit not to run that collection in parallel with any other.

If the issue was that the documentation tests were running in parallel with the MS tests, and failing because the tests were colliding, then your commit should fix it. You might also have chosen just to change the [Collection] attribute on the documentation tests to Mssql instead of MssqlTests. This would have put the tests into the same collection as the other MS tests, and none of those tests would then run in parallel with each other.

[Ste1io]

You might also have chosen just to change the [Collection] attribute on the documentation tests to Mssql instead of MssqlTests. This would have put the tests into the same collection as the other MS tests, and none of those tests would then run in parallel with each other.

As a correction to my last comment, they were previously in "MssqlTests", which differs from the other mssql tests which reside in a "Mssql" collection. Regardless, your point that including in the same collection would also fix the issue confirms what I was asking about.

In this case, keeping them in a separate "Documentation" collection is probably best, since those tests are documenting general library usage, and not specific to mssql aside from that happening to be the provider used when they were written. Ultimately, those tests need revisited anyways (hard-coding sql escapes isn't compatible with all dbms supported by the library, and example test cases really should employ best practices).

[Ste1io]

Was there a specific reason for using a MEMO data type for the MS Access db People.Id column, instead of GUID? The mapped POCO property is System.GUID.

https://github.com/CollaboratingPlatypus/PetaPoco/blob/35b5286f66988b5a8c895d2f1eaf85bc40c2b987/PetaPoco.Tests.Integration/Scripts/MSAccessBuildDatabase.sql#L10-L16

TEXT might make sense I suppose, but MEMO seems a bit overkill. Also, MS Access doesn't allow joins on MEMO columns - one of two contributing factors for the two failing tests with this database.

Also, turns out that in addition to changing the data type to GUID, explicitly using the INNER JOIN syntax was necessary for the tests to pass. Even though an implicit JOIN is supposedly no different??? Microsoft is so freaking weird man I swear... Everything they do they gotta do it different just because.

[Curlack]

Lol. I know the feeling man. Hang in there.

On Fri, 22 Sep 2023, 5:45 pm Stelio Kontos @.***> wrote:

Was there a specific reason for using a MEMO data type for the MS Access db People.Id column, instead of GUID? The mapped POCO property is System.GUID.

https://github.com/CollaboratingPlatypus/PetaPoco/blob/35b5286f66988b5a8c895d2f1eaf85bc40c2b987/PetaPoco.Tests.Integration/Scripts/MSAccessBuildDatabase.sql#L10-L16

TEXT might make sense I suppose, but MEMO seems a bit overkill. Also, MS Access doesn't allow joins on MEMO columns - one of two contributing factors for the two failing tests with this database.

Also, turns out that in addition to changing the data type to GUID, explicitly using the INNER JOIN syntax was necessary for the tests to pass. Even though an implicit JOIN is supposedly no different??? Microsoft is so freaking weird man I swear... Everything they do they gotta do it different just because.

— Reply to this email directly, view it on GitHub https://github.com/CollaboratingPlatypus/PetaPoco/pull/696#issuecomment-1731642424, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACI4HPMMP7N7X7VBIM2UETX3WXDFANCNFSM6AAAAAA47TURS4 . You are receiving this because you commented.Message ID: @.***>

[Ste1io]

Lol. I know the feeling man. Hang in there.

I really should be used to it by now, after 10 years of putting up with their antics. 🤣

Only one dbms left to go, though, so nearly there. Can't wait to find out what kind of surprises they managed to pack into Compact Edition.

[Ste1io]

@pleb I've installed SSCERuntime_x64-ENU.exe, and hitting a hard stop from the MssqlCeDBTestProvider constructor: "System.UnauthorizedAccessException : Access to the path 'C:\Windows\assembly\GAC_MSIL\System.Data.SqlServerCe\4.0.0.0__89845dcd8080cc91\sqlceca40.dll' is denied."

https://github.com/CollaboratingPlatypus/PetaPoco/blob/7e6c9401adb011fd8f4b5f07e8210001da529f2b/PetaPoco.Tests.Integration/Databases/MSSQLCe/MssqlCeDBTestProvider.cs#L15-L31

MSSQL Compact Edition is the only dbms I'm unable to test at this point; all remaining tests pass with the changes in this branch via docker and a local MS Access env.

Is there no alternative way of using the right dlls? Why not just add an updated package to nugget?

[Ste1io]

Update on sqlce: I modified the test provider ctor to copy the files into the executable's directory, and it times out during connection attempt.

I've literally never messed with SQL Crappy Edition before, and not finding anything very useful online either; getting to the point where idgaf about that provider unless someone jumps in with something super helpful here.

[pleb]

Regarding MS access and MS CE, I wouldn't worry too much about making these work in CI/CD. It would be nice, but I suspect few people use PetaPoco against Access and CE.

Regarding CE, maybe we should remove support as MS themselves no longer support it https://learn.microsoft.com/en-us/lifecycle/products/microsoft-sql-server-compact-40

[Ste1io]

Freakin awesome.

Tbf, I'm actually keen on maintaining support for MS Access... With a little bit of fiddling, I should be able to get it to run inside a custom docker setup. And I know people who still use it. Haven't checked on the official status though.

Regardless, I 100% agree that the CI/CD pipeline should be able to run all tests, so ultimately that will be the deciding factor. IMHO, having the tests easily ran on a local developer's workstation with minimal setup via docker is essential also; having to push to remote just to run the tests gets messy real fast. Messier than even my notoriously messy commits 😂.

I'm about half finished with moving tests out to base that aren't dbms-specific. I also added a BaseTriageTests class (lmk if you prefer another name) to serve as an initial "catch all" location for bug investigations to go initially when doing a quick repro, prior to becoming regression tests and deciding where they should live long term. Should keep the permanent tests clean and organized, and remove the analysis paralysis from happening when a new contributor wants to open a PR, but is unsure of where it should go or where to start.

Have you had a chance to review everything yet, or any other feedback? I should have my checklist completed by the end of the day; eager to get this in the dev branch so it's not slowing things down anymore.

[Edit] @pleb, what's the naming convention being used for bug investigations table names? The hash suffix doesn't seem to match up with anything, that or I'm just blind.

[Ste1io]

Building and Development wiki page updated with current updated info on development environment and testing setup.

[Ste1io]

The lack of distinction between database systems (such as MSSQL) and drivers/libraries (such as MSSQL MsData) in the tests is resulting in a significant mixture of duplicate code. They already use shared data, any reason not to share tests also? Currently, models are duplicated, all identical tests are duplicated, yet both share the same build scripts and underlying database systems.

It would make more sense to me if drivers inherited from a common database test class (no different from the current setup, except one additional layer of abstraction). This would allow driver-specific tests to be easily differentiated from dbms specific tests, also.

This will make adding tests for Oracle easier as well.

I already implemented this change for MSSQL locally with no issues or test failures. Any reservations on me implementing this change to the integration test suite @pleb @asherber?

[asherber]

I agree in general that different drivers for the same dbms should share tests, rather than duplicate.

[Curlack]

SELECT FROM [Orders] o WHERE o.[Id] = @0; SELECT FROM [People] p WHERE p.[FullName] = @1;

I think the error is due to the text after semicolon.

On Wed, 27 Sep 2023, 7:46 pm Stelio Kontos @.***> wrote:

@.**** commented on this pull request.

In PetaPoco.Tests.Integration/Databases/MSAccess/MsAccessQueryTests.cs https://github.com/CollaboratingPlatypus/PetaPoco/pull/696#discussion_r1338985725 :

+

• order.PoNumber.ShouldStartWith("PO");
• order.Status.ShouldBeOneOf(Enum.GetValues(typeof(OrderStatus)).Cast().ToArray());
• order.JoinablePersonId.ShouldNotBe(0);
• order.CreatedOn.ShouldBeLessThanOrEqualTo(new DateTime(1990, 1, 1, 0, 0, 0, DateTimeKind.Utc));
• order.CreatedBy.ShouldStartWith("Harry");
• order.JoinablePerson.ShouldNotBeNull();
• order.JoinablePerson.Id.ShouldNotBe(0);
• order.JoinablePerson.Name.ShouldStartWith("Peta");
• order.JoinablePerson.Age.ShouldBe(18);
• }
• // FIXME: System.Data.OleDb.OleDbException : Characters found after end of SQL statement.
• [Fact]
• public override void QueryMultiple_ForMultiResultsSetWithSinglePoco_ShouldReturnValidPocoCollection()

Not sure what the issue is here...generated SQL is:

SELECT FROM [Orders] oWHERE o.[Id] = @0;SELECT FROM [People] pWHERE p.[FullName] = @1;

— Reply to this email directly, view it on GitHub https://github.com/CollaboratingPlatypus/PetaPoco/pull/696#pullrequestreview-1647259945, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACI4HP52HJBQHTTPN6GYCTX4RQ73ANCNFSM6AAAAAA47TURS4 . You are receiving this because you commented.Message ID: @.***>

[Ste1io]

I think the error is due to the text after semicolon.

Meaning, same problem as Firebird? eg: only one statement allowed per command?

[Ste1io]

@pleb

[Curlack]

It seems so. Saw the same problem here: https://www.codeproject.com/Questions/1137585/How-do-I-select-data-from-multiple-table-in-single They solved it using DataAdapter.Fill, not sure if we can utilize it somehow. Otherwise, yes, same as Firebird, although I'm not familiar with it 😁

You've done one hell of a job with this code base, well done man! I'm thinking of setting everything up on my side and jump in there with you whenever I can get some spare time...

On Wed, 27 Sep 2023, 7:56 pm Stelio Kontos @.***> wrote:

I think the error is due to the text after semicolon.

Meaning, same problem as Firebird? eg: only one statement allowed per command?

— Reply to this email directly, view it on GitHub https://github.com/CollaboratingPlatypus/PetaPoco/pull/696#issuecomment-1737842565, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACI4HMY7RKC5CNSWOA5ANTX4RSDBANCNFSM6AAAAAA47TURS4 . You are receiving this because you commented.Message ID: @.***>

[Ste1io]

I'm thinking of setting everything up on my side and jump in there with you whenever I can get some spare time...

You should... Will be much easier once this gets merged. Currently working on a table data delete feature to add to tests that maps out the constraints from the sql meta and deletes the data bottom-up. Not having to run the build scripts with a total teardown and create for each test should speed up the tests significantly.

And been itching to add some features I've been using myself for awhile now, but couldn't really bring to the table without functioning tests. Like my PocoDataHelper for typesafe inline escaping with intellisense support, with next to zero overhead (no expression compilation and built-in caching).

Appreciate the support and input btw. First time messing with MS Access, CE, Firebird, and Postgres. Always fun to figure out new systems you hope you'll never use again 😂 though I got to say, very impressed with Postgres's performance and infra; definitely going to mess around with it more.

I'll revisit the data sets and DataAdapter.Fill mentioned in your link. It was my understanding that only worked with VBA, but I could be wrong.

[pleb]

Love your work, @Ste1io