Efcore: Make FromSqlSprocQueryTestBase more resilient to exceptions

[lauxjpn]

Adding another (non client eval) test to the FromSqlSprocQueryTestBase with a Task<Exception> return type in the future, would likely be missed by providers, because the test would be green by default even when it actually fails.

Consider changing all Task<Exception> return types to Task, and just throw the exceptions instead.

[roji]

Throwing would mean that tests fail by default, and providers must override to make them pass, which isn't our standard pattern.

However, the base implementation can assert that the exception is DbException before returning it.

[lauxjpn]

Wouldn't the test only fail, if they are supposed to?

Let's take From_sql_queryable_stored_procedure_composed as an example:

https://github.com/aspnet/EntityFrameworkCore/blob/a19645e06aa02db58300248d4df652832e3cbfb0/test/EFCore.Relational.Specification.Tests/Query/FromSqlSprocQueryTestBase.cs#L141-L165

If this test would be added in the future and not overridden by the provider, we have the following outcomes:

If the generated SQL query is supported by the underlying database provider, the query would not throw (and then return the exception) but would succeed (and then return null), which is correct and expected. (@roji This should address your scenario.)

On the other hand, if the generated SQL query is not supported by the underlying database provider, with the current implementation the query would throw an exception which would be caught in the base class, and then returned. Because the base class was not overridden, the returned exception is not being handled up the stack and the test falsely succeeds.
With the proposed implementation, the exception would not be caught before xUnit would handle and report it, resulting in the test failing, which is correct and expected.

Failing on a not supported query is the default in other parts of the test suite and should fail, when the base class implementation of the test has not been overridden.

[roji]

The point is that the current tests returning Task are definitely supposed to fail in (most) relational databases - you simply cannot compose over a stored procedure call. Therefore, when the base class isn't overridden, if a DbException is generated from the ADO provider, we have the expected behavior and the test can succeed. We do not want providers to have to override these tests, since again, failure is the expected behavior and there's no reason to force providers to assert anything provider-specific here (although they may, just as SQL Server already does).

These questions aside, I'm not sure how much value it adds to assert for these exceptions, but that's a different question.

[lauxjpn]

Okay, that makes more sense to me now. So the expected behavior is for the tests to fail.

In that case, this issue can probably be closed.

It could be argued, that the fact that these are negative test should be reflected in the test names and that the base implementation should fail, if there is actually support for the test available, forcing the provider to override the test, but this is exactly the valid point of @roji, that this would deviate from the default handling of success cases in the test suite.
On the other hand, me understanding only now the actual intent of the tests, might be a hint that it is maybe not a bad idea to make them more explicit.

[roji]

I do find the current tests problematic in that they test for any exception to be thrown, which is a general anti-pattern - I'm repurposing this issue for that.