316 assert functions

[michalporeba]

As you are probably aware I struggled through the weekend with testing the checks. Then certain SQL DBA with a Bear(d) pointed me to http://jakubjares.com/2017/12/07/testing-your-environment-tests/ (thank you, you know who you are ;) )

In this PR I have

• implemented a unit test for the database collation test.
• removed the two integration tests for checks I have added previously.
• created new files to make it all work.

Before we merge this PR I'd like to discuss the new files, namely the internal/checks/Database.ps1 and previously created internal/functions/Get-DatabaseDetail.ps1.

In Jakub's article he keeps the Assert-* function next to the tests. In my opinion it increases code readability as at a quick glance one knows what the check is actually testing. The problem with this is, that you must (as far as I can tell) keep the check tests in the same file as otherwise you will have to include (dot reference) the checks file from the check tests file which is executed by pester, which triggers all of the checks. To solve that problem I have created internal/checks/Database.ps1 files where I intend to place all the Assert-* function and include that file in both checks/Database.Tests.ps and tests/Database.Tests.ps1. That is how it is right now and it works.

But now the checks are effectively spread across 3 files: the checks, the assert function and the interna/functions/Get-DatabaseDetail.ps1 which will only ever be used in the database checks. So I was thinking, wouldn't it make sense to put it in the internal/checks/Database.ps1 with the Assert-* functions?

What do you think?

[SQLDBAWithABeard]

I need to look at this when I have a little more time. I would rather keep the functions in separate files to enable reuse

[michalporeba]

I've added few more tests so it is more obvious what functions we will have. Function reuse is obviously a good thing to have, but I don't see how the Assert-* functions (currently in internal/checks/Database.ps1) would be reused outside of the checks file to and the checks tests. I don't think we would benefit from having one file per function there. Perhaps it would be more 'natural' from the software development point of view, but at the same time we have only one checks file with multiple checks and here relation appears to be one to one.

My argument with Get-DatabaseDetail (perhaps the name could be changed) is only ever to be used in the database tests. If we had better scopes in PowerShell I would put all the Assert-* and the Get-DatabaseDetail inside the Database.Tests.ps1 as private functions.

[michalporeba]

I brought this branch up to date with the latest developments in the development branch. Any comments on the way we want to go with this? I'm eager to write more tests and make the checks faster but there is the question about reuse. @SQLDBAWithABeard, @potatoqualitee, @AnyBody?

[SQLDBAWithABeard]

apologies for the delay, I need to sit with a clear head for this one to make sure we go the right way forward

[potatoqualitee]

The only thing that concerns me here is this reduces the ease of contributing. But I do love the speed increase you mentioned. I'm just not sure how to make it easier for the casual developer. Should the impacted tests have some code comments above to ease people in?

[michalporeba]

So I've been thinking... here is how I see it today

We have number of (somehow conflicting) requirements

1. Check files should contain nothing but the checks
2. All checks should be unit tested
3. Ease of access for new contributors
4. 'Proper' structure to promote reuse (DRY)
5. Internal functions need to be in internal/functions
6. Each file should contain only one function

The above are in the order of priority as I see it (today). Also, to be able to implement number 2 above we need to use Assert-* functions for each check which cannot be defined in the check file (e.g. Database.Tests.ps1) because otherwise we would be executing checks when running their unit tests - that's how Pester works.

Currently the checked in code structure was mostly influenced by points 1, 2, 5 and to some extent 4 making it a bit confusing for new contributors. To increase accessibility I propose we do the following

1. in the checks folder for each group of checks (currently one *.Tests.ps1 file) we should have two files, one for the pester tests and one for the corresponding assert functions. So for database checks we sould have Database.Asserts.ps1 and Database.Tests.ps1. I think it would be very easy to explain that convention.
2. Additionally for each group we would have unit tests file in 'tests/checks' named in our example Test.Database.Asserts.ps1 because the assert functions is what we actually are unit testing.
3. In each of the 3 related files we would add some comments at the top explaining the relations between the 3 files
4. Perhaps most controversially I would put functions like Get-DatabaseDetail (currently in its own file) at the end of checks/Database.Asserts.ps1 file as it should really be only used by those assert functions and nowhere else.

@potatoqualitee I think that would make the structure easy to explain with logical split of code between three files per group of checks.
@SQLDBAWithABeard the Assert functions are in their own file and they can be reused in the only two places where they should be reused - the checks file and unit tests file. The Get-DatabaseDetail is really an internal, a private method for the group of tests and trying to make it reusable in a broader scope might make it unnecessarily complex at the same time making the code less straight forward for new people who probably would like to focus on checks, not on the internal functions.

What do you think?

/checks/
| Database.Asserts.ps1
| Database.Checks.ps1
/tests/checks/
| Database.Assertions.Tests.ps1

that is how it would look like

[michalporeba]

I have reorganised the files adding some comments to, just to illustrate my suggestions from the previous comment. Any thoughts? There is still the matter of where the Get-DatabaseDetail go which I haven't addressed in those latest changes, and I will need to spend some more time testing before this branch can be merged, but I hope this change helps in the discussion

[SQLDBAWithABeard]

Thats a perfect place for get-databasedetail

[michalporeba]

Please have a look at the page verify and auto close checks. I have just realised that with the proposed structure it is easy to validate configuration parameters to avoid surprises with checks passing (or failing) because of a typo in the config.

It will also allow to cleanup the configuration keys like policy.database.autoclose and policy.pageverify. That was something I was thinking about recently, I'd like to have both either with or without that database part. With this approach in Get-SettingsFor*Check we can check if the old key was provided, get the value and display a warning that it has been deprecated. It is not something I want to do now, but a new possibility I have just realised opened up with this approach of having the Assert and Get-SettingsFor function.

Lastly (for this comment) I would like to discuss the file structure. Keeping the checks in as few files as possible might make sense because of how the Pester works, or how easy it is to browse through them. But the unit tests file? It is becoming less and less manageable with the Database.Asserts.Tests.ps1 file growing very quickly. What do you think about splitting it into independent files like Database.AutoClose.Tests.ps1, Database.PageVerify.Tests.ps1 ? I was also thinking whether it would make sense to split the Database.Asserts.ps1 into Database.AutoClose.ps1, Database.PageVerify.ps1 and put them into an Asserts folder. It could make working with the files much easier while still keeping only 1 Database.Tests.ps1 file in the checks folder. Any thoughts?

[michalporeba]

Now with the reduced 'the sea of green' effect, could we perhaps consider splitting the Database.Tests.ps1? It is a very long file, and perhaps unnecessarily so. I see at least three categories of checks there, the database configuration, the backups which are very different and likely to be tested on a different schedule (and validation might take long time) and there are checks about data itself (identity check). I don't really know at the moment where the index checks sit, and if we wanted to add checks validating the constraints or triggers are enabled where would those go. But it looks like there is potential for many more checks in an overall 'Database' category, which might make the test file very, very difficult to work with.

I'm bringing it up here, because I think the backup checks will be a bit out of scope for what I'm doing here in this PR, and splitting the file, even if only by extracting the backup checks, would make it easier to eventually merge all those changes.

What do you think?

[michalporeba]

Issue #435 made me think about supporting older versions of SQL Server. I can only test 2008 R2 to 2017. For the changes I have implemented so far it won't be very difficult as I will simply use the sys.sysdatabases which is still available for backwards compatibility, but we'll have to think how to test things like that.

I thnk I will have to implement this change before we think about merging this PR

the code has significantly changed since the review.

[michalporeba]

So here it is, I think it is time to close this very long standing PR. I opened it almost 6 weeks ago. Since then there was a lot of discussions around the idea of how to better validate the checks, 66 commits, I had 4 different implementations (3rd one is the winner if anybody cares). It was very helpful for me, and I hope it wasn't too annoying for you. (And I must say I really liked how those long PRs allow for frequent code reviews and collaboration, so watch out!)

But the time has come to close it down. As agreed with @SQLDBAWithABeard at some point next week I will create a new branch directly in sqlcollaborative rather than in my own fork, and merge all the changes there. Then I will ask for a few volunteers to help to test it before the new PR will be created and hopefully at some point in April we'll get it out.