dotnet / dotnet/SqlClient

Use RAII primitives to create database objects in existing tests

Open
#4,419 1 comment 0 reactions 2 assignees Claimed by @mdaigle View on GitHub
Approval Needed Area\Tests
Dominant language
C#
Stars
989
Forks
340
Avg merge
4d 19h
Merged PRs (30d)
72

Description

In #4050, we introduced a set of RAII primitives. These objects would create SQL objects when instantiated, then drop them upon disposal. This reduces the likelihood of new tests leaving tables behind on the shared database server in unexpected circumstances.

Since then, subsequent PRs which introduce new integration tests (#4234, #4196, #3677, #4262, #4306) have made use of them. #4306 has also expanded them slightly. As a result, we now have objects for these database-/server-level DDL objects:
* Table
* Stored procedure
* User-defined type
* User
* Login

This issue tracks the replacement of existing SQL object creation/cleanup logic with these objects (or similar objects.) There's currently a handful of approaches, and almost all of them should be removed.
1. Calls to `DataTestUtility.CreateTable` / `DataTestUtility.CreateSP` (sometimes paired with the corresponding `DropTable` and `DropStoredProcedure` methods)
2. Calls to `Helpers.TryDropTable` and `Helpers.TryExecute`
3. Test-specific helper methods which fill the same purpose as 1 & 2
4. Test-specific fixtures which run the `CREATE` statements
5. Manual execution of `CREATE` statements
6. Fixtures which create Always Encrypted-enabled tables with various hardcoded schemas

The existing primitives will likely need adjustments as this proceeds - we'll likely need to add new primitives for the `CREATE QUEUE` and `CREATE SERVICE` statements, and there are a few instances where the SqlCommand which runs the creation statement needs to have an associated transaction. Some primitives will need to reference or wrap others (such as an Always Encrypted-enabled table, which will need to reference a Column Encryption Key, which will in turn need to reference a Column Master Key, which will in turn need to wrap a certificate.)

There are also a small number of recurring situations which result in sets of objects being created for the same purpose. In SqlBulkCopy tests, this pattern often involves creating a source and a target table. In such a scenario, I think it makes sense to create a second set of classes: `Scenario`s. This should implement `IDisposable` and consist of multiple primitives. Instantiating an instance of a `Scenario` class would safely create the associated objects, (safely rolling back if the creation fails part-way through) and disposing it would drop them in reverse order.

PRs to perform the remediation will likely touch a large number of files. The general principles I'm keeping in mind are:
* Minimise whitespace changes - even if this means that we're nesting `using (...) { ... }` blocks
* Avoid changes to tests which are covered by existing open PRs
* Break the files/tests down by commit
* Keep all other test functionality identical unless it's completely unavoidable

The process of remediating the existing object creation logic is likely to have a long tail. Linked PRs are below, roughly grouped into categories:
1. Create primitives: #4050
2. Expand set of available primitives: #4306, TBC
3. Handle common, standalone creation of objects: TBC
4. Introduce and use a `BulkCopyScenario` object: TBC
5. Address the Always Encrypted integration tests: TBC

Once all five buckets are complete, we should have a very limited number of manual DDL object creation statements and it should be clear why each occurrence exists.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.