Efcore: Microsoft.Data.Sqlite: Leverage ADO.NET spec tests

[bricelam]

Some progress on dotnet/corefx#7810 has been made in mysql-net/AdoNetApiTest. We should consider leveraging it here.

cc @bgrainger @roji

[bgrainger]

An example of consuming the current NuGet package (and overriding its behaviour) for MySQL is: https://github.com/mysql-net/MySqlConnector/tree/master/tests/Conformance.Tests

[roji]

I'd be interested in working on this, but does it really belong in EFCore as opposed to the individual providers, or possibly corefx?

[bricelam]

@bgrainger Didn't you have a table somewhere of which providers fail what tests? I'm starting my 3.0 work and want to look at what breaking changes we should make to Microsoft.Data.Sqlite...

[bgrainger]

results.zip

I've attached the latest "results" file generated locally on my machine. (The zip file should be one self-contained HTML file of the results.) Unfortunately there's no automatic CI build or a website where updated results are published.

I tried updating the Microsoft.Data.Sqlite tests to 2.1.0 but just got a wall of Could not load file or assembly 'Microsoft.Data.Sqlite, Version=2.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' exceptions with dotnet xunit. dotnet test worked fine, though, so I might have to work on mysql-net/AdoNetApiTest#9.

[bgrainger]

results.zip

Updated results with Microsoft.Data.Sqlite 2.0.0 and 2.1.0 side-by-side (columns 3 & 4).

Most obvious changes are an exception from Dispose_command_before_reader (aspnet/Microsoft.Data.Sqlite#484) and GetX throwing InvalidOperationException (change from InvalidCastException) for NULL columns.

[roji]

@bgrainger this is really great work :) Is there a place where provider-specific work can happen, e.g. flag which tests are inapplicable to the provider as opposed to failing?

[bgrainger]

To fix the comparison chart itself, individual tests can be overridden on a per-provider level by modifying the provider-specific test project, e.g., https://github.com/mysql-net/AdoNetApiTest/blob/master/tests/MySqlConnector.Tests/MySqlConnectorParameterTests.cs. Open a PR or I could make you a contributor.

If you want to add the tests into your own test suite (in npgsql), see https://github.com/mysql-net/AdoNetApiTest#how-to-use; specific examples of overriding behaviour in MySqlConnector are at https://github.com/mysql-net/MySqlConnector/blob/master/tests/Conformance.Tests/ConnectionTests.cs and https://github.com/mysql-net/MySqlConnector/blob/master/tests/Conformance.Tests/GetValueConversionTests.cs.

[roji]

@bgrainger thanks! I think these tests belong in individual provider repos, rather than in a single unified place - this way they can evolve with the provider etc. Hopefully we'll have some time to work on this in Npgsql (see https://github.com/npgsql/npgsql/issues/2225).

[bgrainger]

Agreed about tests living with the provider; the test applications in AdoNetApiTest are mostly for me to easily check other libraries' behaviour when making design decisions for MySqlConnector; see https://github.com/dotnet/corefx/issues/7810#issuecomment-346109151.

[ajcvickers]

@roji Let's talk about this in our call.

[ajcvickers]

@divega I created "shay-as-a-label" so we can tag things now.

[bgrainger]

I'm happy to add anyone as a collaborator to https://github.com/mysql-net/AdoNetApiTest if you think it's a good base and want to contribute more to it; just reply here or open an issue.

Or we could transfer the repo to the aspnet organisation if this would be a better long-term home.

[roji]

Whoa, I'm a label now...

[AlexanderTaeschner]

I just ran the tests on the current master (see this branch). The main issues (in addition there are about 100 tests in the GetValueConversionTest category which one has to go through in addition) were:

---

SqliteConnection

Differences in behavior

• SqliteConnection.Open cannot be called twice (Open_cannot_be_called_twice)
• SqliteConnection.CreateCommand creates a command where the Transaction property is not null (CreateCommand_does_not_set_Transaction_property)

SqliteTransaction

Differences in expected exception types

• SqliteTransaction.Commit throws InvalidOperationException instead of ObjectDisposedException after SqliteTransaction.Dispose is called (Commit_transaction_throws_after_Dispose)
• SqliteTransaction.Rollback throws InvalidOperationException instead of ObjectDisposedException after SqliteTransaction.Dispose is called (Commit_transaction_throws_after_Dispose)

SqliteDataReader

Differences in behavior

• SqliteDataReader.Read throws when the generating command is already disposed (Dispose_command_before_reader)
• SqliteDataReader.NextResult does not throw (InvalidOperationException) when reader is already disposed (NextResult_throws_when_closed)
• SqliteDataReader.GetBytes throws when dataOffset is too long instead of returning 0 (GetBytes_reads_nothing_when_dataOffset_is_too_large)
• SqliteDataReader.GetChars throws when dataOffset is too long instead of returning 0 (GetChars_returns_length_when_buffer_is_null)
• SqliteDataReader.GetChars throws when buffer is null instead of returning the length (GetChars_returns_length_when_buffer_is_null)
• SqliteDataReader.GetValues throws when values is too small instead of returning 0 (GetValues_when_too_narrow)

Differences in expected exception types

• SqliteDataReader.GetBytes throws ArgumentException instead of ArgumentOutOfRangeException when bufferOffset + length are too long (GetBytes_throws_when_bufferOffset_plus_length_is_too_long)
• SqliteDataReader.GetDataTypeName throws ArgumentOutOfRangeException instead of IndexOutOfRangeException when ordinal is out of range (GetDataTypeName_throws_when_ordinal_out_of_range)
• SqliteDataReader.GetFieldType throws ArgumentOutOfRangeException instead of IndexOutOfRangeException when ordinal is out of range (GetFieldType_throws_when_ordinal_out_of_range)
• SqliteDataReader.GetName throws ArgumentOutOfRangeException instead of IndexOutOfRangeException when ordinal is out of range (GetName_throws_when_ordinal_out_of_range)
• SqliteDataReader.GetOrdinal throws ArgumentOutOfRangeException instead of IndexOutOfRangeException when ordinal is out of range (GetOrdinal_throws_when_out_of_range)

SqliteParameter

Differences in behavior

• SqliteParameter.ParameterName throws ArgumentNullException when set to null (ParameterName_can_be_set_to_null)
• SqliteParameter.SourceVersion default value is Default instead of Current (Parameter_default_SourceVersion_is_Current)

---

The question is how to deal with those test failures: should I file an issue for them or can someone of the EF team go through them.

[bricelam]

should I file an issue for them or can someone of the EF team go through them

I think this issue is also about making Microsoft.Data.Sqlite conform better.

Some of these will be addressed as we rework the architecture in 3.0 (motivated by issues like #13826, #13830, #13836, #13837 & #14044)

[bgrainger]

Regarding the conformance tests, there are three possibilities for each test failure:

• The conformance tests are wrong. In many cases, the documentation is vague and doesn't prescribe a particular behaviour, particularly when handling an error condition. So when writing them, I chose to enforce a particular behaviour that either seemed logical, aligned with SqlClient behaviour, or seemed to be the de facto standard across many different providers. In certain cases, I may have chosen wrongly; this would be a bug to open on the conformance tests themselves (to change the expected output).
• Microsoft.Data.Sqlite is different, but not "wrong": you can argue a good case for why it differs from the "standard" in the conformance tests. In this case, you fix the problem by overriding the test method and asserting your different behaviour instead, e.g., as is done here for MySqlConnector: https://github.com/mysql-net/MySqlConnector/blob/717c3ecaf9582ab106d586180350e3b690756594/tests/Conformance.Tests/GetValueConversionTests.cs#L14-L19. It's very likely there might be a lot of these kind of issues due to SQLite's loose typing, which can be hard to map onto ADO.NET's API.
• Microsoft.Data.Sqlite is wrong. In this case, it would be appropriate to open a bug reporting the failure, and fix the ADO.NET code. However, this might introduce backwards compatibility concerns if it changes visible behaviour.

But a conformance test failure doesn't immediately imply a bug in Microsoft.Data.Sqlite that needs an issue to be filed.

[bricelam]

I finally looked through each of the test results for Microsoft.Data.Sqlite. I wasn't concerned by our differences in negative scenarios, but I filed #14682 to address some failures that I do think are bugs in our provider.