dataplat / dbatools

🚀 SQL Server automation and instance migrations have never been safer, faster or freer
https://dbatools.io
MIT License
2.39k stars 787 forks source link

ConvertTo-DbaDataTable handle Dataplat.Dbatools.Utility.DbaDateTime as an array #9361

Closed jpomfret closed 1 month ago

jpomfret commented 1 month ago

Type of Change

Commands to test

Import-Module ./dbatools.psd1 -force
$cred = Get-Credential sqladmin
$inst = Connect-DbaInstance -SqlInstance localhost -SqlCredential $cred
$Results = Test-DbaLastBackup -SqlInstance $inst -Destination $inst -EnableException 
$test = ConvertTo-DbaDataTable -InputObject $Results
wsmelton commented 1 month ago

Fascinated how this test is passing because this test fails as I expect it to on my local machine

image
$innedobj = New-Object -TypeName psobject -Property @{
        Mission = 'Keep Hank alive'
    }

    Add-Member -Force -InputObject $obj -MemberType NoteProperty -Name myobject -Value $innedobj

The above sets myObject to a value (note that name should be camelCasing 😜) so this test is either false or we shouldn't be setting it to a value at all and just creating the property?

wsmelton commented 1 month ago

Just a few notes on this test as a whole, not something you must change unless you just want to include it in this PR...

image

_The last item is also being done twice in this test for some reason, contexts Property: dbadatetimeArray and Property: myObject are both testing that object. Maybe remove one of them if it is really duplicate?

All of this just makes the test a bit easier to read. A version with the above changes is attached if you want to compare it all in VS Code more easily:

$CommandName = $MyInvocation.MyCommand.Name.Replace(".Tests.ps1", "")
Write-Host -Object "Running $PSCommandPath" -ForegroundColor Cyan
. "$PSScriptRoot\constants.ps1"

Describe "$CommandName Unit Tests" -Tag 'UnitTests' {
    Context "Validate parameters" {
        [object[]]$params = (Get-Command $CommandName).Parameters.Keys | Where-Object {$_ -notin ('WhatIf', 'Confirm')}
        [object[]]$knownParameters = 'InputObject', 'TimeSpanType', 'SizeType', 'IgnoreNull', 'Raw', 'EnableException'
        $knownParameters += [System.Management.Automation.PSCmdlet]::CommonParameters
        It "Should only contain our specific parameters" {
            (@(Compare-Object -ReferenceObject ($knownParameters | Where-Object {$_}) -DifferenceObject $params).Count ) | Should Be 0
        }
    }
}

Describe "Testing data table output when using a complex object" {
    $obj = New-Object -TypeName psobject -Property @{
        guid             = [system.guid]'32ccd4c4-282a-4c0d-997c-7b5deb97f9e0'
        timespan         = New-TimeSpan -Start 2016-10-30 -End 2017-04-30
        datetime         = Get-Date -Year 2016 -Month 10 -Day 30 -Hour 5 -Minute 52 -Second 0 -Millisecond 0
        char             = [System.Char]'T'
        true             = $true
        false            = $false
        null             = [bool]$null
        string           = "it's a boy."
        UInt64           = [System.UInt64]123456
        dbadatetime      = [dbadatetime[]]$(Get-Date -Year 2024 -Month 05 -Day 19 -Hour 5 -Minute 52 -Second 0 -Millisecond 0)
        dbadatetimeArray = [dbadatetime[]]($(Get-Date -Year 2024 -Month 05 -Day 19 -Hour 5 -Minute 52 -Second 0 -Millisecond 0), $(Get-Date -Year 2024 -Month 05 -Day 19 -Hour 5 -Minute 52 -Second 0 -Millisecond 0).AddHours(1))
    }

    $innedobj = New-Object -TypeName psobject -Property @{
        Mission = 'Keep Hank alive'
    }

    Add-Member -Force -InputObject $obj -MemberType NoteProperty -Name myObject -Value $innedobj
    $result = ConvertTo-DbaDataTable -InputObject $obj

    Context "Property: guid" {
        It 'Has a column called "guid"' {
            $result.Columns.ColumnName | Should -Contain 'guid'
        }
        It 'Has a [guid] data type on the column "guid"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'guid' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'guid'
        }
        It 'Has the following guid: "32ccd4c4-282a-4c0d-997c-7b5deb97f9e0"' {
            $result.guid | Should Be '32ccd4c4-282a-4c0d-997c-7b5deb97f9e0'
        }
    }

    Context "Property: timespan" {
        It 'Has a column called "timespan"' {
            $result.Columns.ColumnName | Should -Contain 'timespan'
        }
        It 'Has a [long] data type on the column "timespan"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'timespan' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'Int64'
        }
        It "Has the following timespan: 15724800000" {
            $result.timespan | Should Be 15724800000
        }
    }

    Context "Property: datetime" {
        It 'Has a column called "datetime"' {
            $result.Columns.ColumnName| Should -Contain 'datetime'
        }
        It 'Has a [datetime] data type on the column "datetime"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'datetime' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'datetime'
        }
        It "Has the following datetime: 2016-10-30 05:52:00.000" {
            $date = Get-Date -Year 2016 -Month 10 -Day 30 -Hour 5 -Minute 52 -Second 0 -Millisecond 0
            $result.datetime -eq $date | Should Be $true
        }
    }

    Context "Property: char" {
        It 'Has a column called "char"' {
            $result.Columns.ColumnName | Should -Contain 'char'
        }
        It 'Has a [char] data type on the column "char"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'char' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'char'
        }
        It "Has the following char: T" {
            $result.char | Should Be "T"
        }
    }

    Context "Property: true" {
        It 'Has a column called "true"' {
            $result.Columns.ColumnName | Should -Contain 'true'
        }
        It 'Has a [bool] data type on the column "true"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'true' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'boolean'
        }
        It "Has the following bool: true" {
            $result.true | Should Be $true
        }
    }

    Context "Property: false" {
        It 'Has a column called "false"' {
            $result.Columns.ColumnName | Should -Contain 'false'
        }
        It 'Has a [bool] data type on the column "false"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'false' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'boolean'
        }
        It "Has the following bool: false" {
            $result.false | Should Be $false
        }
    }

    Context "Property: null" {
        It 'Has a column called "null"' {
            $result.Columns.ColumnName | Should -Contain 'null'
        }
        It 'Has a [bool] data type on the column "null"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'null' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'boolean'
        }
        It "Has the following bool: false" {
            $result.null | Should Be $false #should actually be $null but its hard to compare :)
        }
    }

    Context "Property: string" {
        It 'Has a column called "string"' {
            $result.Columns.ColumnName | Should -Contain 'string'
        }
        It 'Has a [string] data type on the column "string"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'string' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'string'
        }
        It "Has the following string: it's a boy." {
            $result.string | Should Be "it's a boy."
        }
    }

    Context "Property: UInt64" {
        It 'Has a column called "UInt64"' {
            $result.Columns.ColumnName | Should -Contain 'UInt64'
        }
        It 'Has a [UInt64] data type on the column "UInt64"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'UInt64' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'UInt64'
        }
        It "Has the following number: 123456" {
            $result.UInt64 | Should Be 123456
        }
    }

    Context "Property: myObject" {
        It 'Has a column called "myObject"' {
            $result.Columns.ColumnName | Should -Contain 'myObject'
        }
        It 'Has a [string] data type on the column "myObject"' {
            $result.myObject | Should -BeOfType [System.String]
        }
        It "Has no value" {
            # not sure if this is a feature. Should probably be changed in the future
            $result.myObject.GetType().FullName | Should Be "System.DBNull"
        }
    }

    Context "Property: dbadatetime" {
        It 'Has a column called "dbadatetime"' {
            $result.Columns.ColumnName | Should -Contain 'dbadatetime'
        }
        It 'Has a [dbadatetime] data type on the column "myObject"' {
            $result.Columns | Where-Object -Property 'ColumnName' -eq 'dbadatetime' | Select-Object -ExpandProperty 'DataType' | Select-Object -ExpandProperty Name | Should Be 'string'
        }
        It "Has the following dbadatetime: 2024-05-19 05:52:00.000" {
            $date = Get-Date -Year 2024 -Month 5 -Day 19 -Hour 5 -Minute 52 -Second 0 -Millisecond 0
            [datetime]$result.dbadatetime -eq $date | Should Be $true
        }
    }

    Context "Property: dbadatetimeArray" {
        It 'Has a column called "myObject"' {
            $result.Columns.ColumnName | Should -Contain 'myObject'
        }
        It 'Has a [string] data type on the column "myObject"' {
            $result.myObject | Should -BeOfType [System.String]
        }
        It "Has the following dbadatetimes converted to strings: 2024-05-19 05:52:00.000, 2024-05-19 06:52:00.000" {
            $string = '2024-05-19 05:52:00.000, 2024-05-19 06:52:00.000'
            $result.dbadatetimeArray -eq $string | Should Be $true
        }
    }

}

Describe "Testing input parameters" {
    $obj = New-Object -TypeName psobject -Property @{
        timespan = New-TimeSpan -Start 2017-01-01 -End 2017-01-02
    }

    Context "Verifying TimeSpanType" {
        It "Should return '1.00:00:00' when String is used" {
            (ConvertTo-DbaDataTable -InputObject $obj -TimeSpanType String).Timespan | Should Be '1.00:00:00'
        }
        It "Should return 864000000000 when Ticks is used" {
            (ConvertTo-DbaDataTable -InputObject $obj -TimeSpanType Ticks).Timespan | Should Be 864000000000
        }
        It "Should return 1 when TotalDays is used" {
            (ConvertTo-DbaDataTable -InputObject $obj -TimeSpanType TotalDays).Timespan | Should Be 1
        }
        It "Should return 24 when TotalHours is used" {
            (ConvertTo-DbaDataTable -InputObject $obj -TimeSpanType TotalHours).Timespan | Should Be 24
        }
        It "Should return 86400000 when TotalMilliseconds is used" {
            (ConvertTo-DbaDataTable -InputObject $obj -TimeSpanType TotalMilliseconds).Timespan | Should Be 86400000
        }
        It "Should return 1440 when TotalMinutes is used" {
            (ConvertTo-DbaDataTable -InputObject $obj -TimeSpanType TotalMinutes).Timespan | Should Be 1440
        }
        It "Should return 86400 when TotalSeconds is used" {
            (ConvertTo-DbaDataTable -InputObject $obj -TimeSpanType TotalSeconds).Timespan | Should Be 86400
        }
    }

    Context "Verifying IgnoreNull" {
        # To be able to force null
        function returnnull {
            [CmdletBinding()]
            param ()
            New-Object -TypeName psobject -Property @{ Name = [int]1 }
            $null
            New-Object -TypeName psobject -Property @{ Name = [int]3 }
        }

        function returnOnlynull {
            [CmdletBinding()]
            param ()
            $null
        }

        It "Does not create row if null is in array when IgnoreNull is set" {
            $result = ConvertTo-DbaDataTable -InputObject (returnnull) -IgnoreNull -WarningAction SilentlyContinue
            $result.Rows.Count | Should Be 2
        }

        It "Does not create row if null is in pipeline when IgnoreNull is set" {
            $result = returnnull | ConvertTo-DbaDataTable -IgnoreNull -WarningAction SilentlyContinue
            $result.Rows.Count | Should Be 2
        }

        It "Returns empty row when null value is provided (without IgnoreNull)" {
            $result = ConvertTo-DbaDataTable -InputObject (returnnull)
            $result.Name[0] | Should Be 1
            $result.Name[1].GetType().FullName | Should Be 'System.DBNull'
            $result.Name[2] | Should Be 3
        }

        It "Returns empty row when null value is passed in pipe (without IgnoreNull)" {
            $result = returnnull | ConvertTo-DbaDataTable
            $result.Name[0] | Should Be 1
            $result.Name[1].GetType().FullName | Should Be 'System.DBNull'
            $result.Name[2] | Should Be 3
        }
    }

    Context "Verifying Silent" {
        # To be able to force null
        function returnnull {
            New-Object -TypeName psobject -Property @{ Name = 1 }
            $null
            New-Object -TypeName psobject -Property @{ Name = 3 }
        }

        It "Suppresses warning messages when Silent is used" {
            $null = ConvertTo-DbaDataTable -InputObject (returnnull) -IgnoreNull -EnableException -WarningVariable warn -WarningAction SilentlyContinue
            $warn.message -eq $null | Should Be $true
        }
    }

    Context "Verifying script properties returning null" {

        It "Returns string column if a script property returns null" {
            $myobj = New-Object -TypeName psobject -Property @{ Name = 'Test' }
            $myobj | Add-Member -Force -MemberType ScriptProperty -Name ScriptNothing -Value { $null }
            $r = ConvertTo-DbaDataTable -InputObject $myobj
            ($r.Columns | Where-Object ColumnName -eq ScriptNothing | Select-Object -ExpandProperty DataType).ToString() | Should Be 'System.String'

        }
    }

    Context "Verifying a datatable gets cloned when passed in" {
        $obj = New-Object -TypeName psobject -Property @{
            col1 = 'col1'
            col2 = 'col2'
        }
        $first = $obj | ConvertTo-DbaDataTable
        $second = $first | ConvertTo-DbaDataTable
        It "Should have the same columns" {
            # does not add ugly RowError,RowState Table, ItemArray, HasErrors
            $firstColumns = ($first.Columns.ColumnName | Sort-Object) -Join ','
            $secondColumns = ($second.Columns.ColumnName | Sort-Object) -Join ','
            $firstColumns | Should -Be $secondColumns
        }
    }
}
jpomfret commented 1 month ago

Fascinated how this test is passing because this test fails as I expect it to on my local machine

Ok I wondered how this was working because it wasn't working for me locally either - I've changed this to make sense (in my mind) and also updated the null one

potatoqualitee commented 1 month ago

oh sweet, we ready? thank you so much! i cant publish till i get home bc of the hardware key but happy to prep with a merge.

potatoqualitee commented 1 month ago

re-read and indeed. thanks again 🙇🏼

potatoqualitee commented 1 month ago

okay that's weird, we have a test that passed but then when it merged, it failed?

Describe : Testing data table output when using a complex object
Context  : Property: myObject
Name     : It Has a [string] data type on the column "myObject"
Result   : Failed
Message  : Expected the value to have type [string] or any of its subtypes, but got  with type [System.DBNull].
potatoqualitee commented 1 month ago

I skipped it in dev since its causing all of our branches to fail

jpomfret commented 1 month ago

Hey @potatoqualitee - I'm in Lisbon to see Taylor swift tonight 😂 but I'll take a look at this! It did pass locally and on the branch before merge 🤔

potatoqualitee commented 1 month ago

HEYYY enjoy taylor swift!!! we can look at this later 😊

jpomfret commented 1 month ago

@potatoqualitee

I created a new branch off develoment and removed the -skip the tests ran fine 🤔 - also testing locally and it runs fine too.

https://github.com/dataplat/dbatools/tree/testingtests

https://ci.appveyor.com/project/dataplat/dbatools/builds/49936347 image

Is it worth uncommenting in development and seeing what happens? If not we can maybe troubleshoot and fix forward?

Honestly it's kind of a weird test anyway - we already test string values get converted ok - so I'm not sure what this is adding.

    Context "Property: myObject" {
        It 'Has a column called "myObject"' {
            $result.Columns.ColumnName | Should -Contain 'myObject'
        }
        It 'Has a [string] data type on the column "myObject"' {
            $result.myObject | Should -BeOfType [System.String]
        }
    }
potatoqualitee commented 3 weeks ago

@jpomfret beautiful! and pfewf. So no change needed to the command, eh? Please do feel free to create a PR and I'll merge with the next batch.

jpomfret commented 3 weeks ago

so @potatoqualitee ... when I went to PR the test was still skipped 🤦

let me try this again - please hold 😂😂

jpomfret commented 3 weeks ago

This is so interesting - this test works completely different in appveyor vs locally

@wsmelton - remember this test - neither of us knew how it was passing in appveyor - I changed it to what was passing locally - and now it's failing in appveyor 😖 https://github.com/dataplat/dbatools/pull/9361#issuecomment-2120920735

image

Shall I remove this test? or change it back to testing for null?

niphlod commented 3 weeks ago

lemme test "on my rig" and get back.

potatoqualitee commented 3 weeks ago

lol thanks niph, i had no idea how to make it pass

niphlod commented 3 weeks ago

yeah. beware that we have another problem, I don't know why "scenario 2008R2, part=1/2" for 2 weeks roughly is sooo borked that it fails badly an the "thingy" that reports back if something fails doesn't know what to read. It seems a pester 5 dependency has slipped in.

https://ci.appveyor.com/project/dataplat/dbatools/builds/49958879/job/0386qjwfw6ofci83 , in particular https://ci.appveyor.com/project/dataplat/dbatools/builds/49958879/job/0386qjwfw6ofci83/tests and image

I don't know how to fix it but we may try to alter appveyor.yml on development to let it clear and re-cache dependencies, as most of them are linked to the content of appveyor.yml itself .

niphlod commented 3 weeks ago

tried my best, AND added some debug prints to uncover the "problem" , see https://ci.appveyor.com/project/dataplat/dbatools/build/job/68qbb1hlj9p98v9w

specifically, I think there might be some "strange happenings" around the fact that:

"ItemArray": [ "System.Collections.Hashtable", true, "T", null, "2024-05-19 05:52:00.000, 2024-05-19 06:52:00.000", null, "32ccd4c4-282a-4c0d-997c-7b5deb97f9e0", "2024-05-19 05:52:00.000", 123456, "it\u0027s a boy.", 15724800000, "\/Date(1477806720000)\/", false, null ]



That being said, bumping appveyor.yml did fix the weird issue of a rogue pester 5 in the cache, and I modified:
- something around manual.pester.ps1 to be more precise 
- appveyor.pester.ps1 now dumps in the log what specific version of pester the test is run with
jpomfret commented 3 weeks ago

Thanks @niphlod for trying - so what do we think for this test, are we thinking it's not really testing what we want? Shall I remove that piece? 🤔

niphlod commented 3 weeks ago

pragmatically ..... something is going awry so if it's the one test causing problems (and, btw, it seems unrelated to serializing succesfully dbadatetime[]) so you may choose to just forget about it. ideally this shows that at least with the combo of software/os/etc on appveyor, it appears that noteproperties are not succesfully serialized as they are "on our rigs".

jpomfret commented 3 weeks ago

Hmm @niphlod - can you check this failure for me? On the firstRow stuff you added in: https://ci.appveyor.com/project/dataplat/dbatools/builds/49972082/job/rw3n9xlu79jedh3j

niphlod commented 3 weeks ago

sure @jpomfret , lemme revert all the "debug" prints, it was just to illustrate that the test fail(ed) because, indeed, that property was null in the first place.

niphlod commented 3 weeks ago

done, it should pass now (and not spit a blob of magenta lines).

jpomfret commented 3 weeks ago

Thank you 👏

niphlod commented 3 weeks ago

BTW, to fix this I'll open another PR (end of day, I promise). It'll include fixes to appveyor.yml, appveyor.pester.ps1 and manual.pester.ps1 so:

They were within the changes I pushed on this branch but they got reverted... better keep this PR "on point" and a separate one will make things better for everyone on the testing side.

niphlod commented 3 weeks ago

I stand corrected, https://github.com/dataplat/dbatools/pull/9384 has everything in.