Attempting to increase reliability of some Pester tests - take 2 (#268)

This is re-implementing #265 in a more robust way, thanks to a suggestion from @X-Guardian in #267. Instead of adding in one-off sleeps throughout the test codebase, there is now a new configuration value `StateChangeDelaySeconds`) that will allow us to insert a delay before returning the result of _any_ state change request. This should ideally consistently add reliability throughout the entire test codebase.
microsoft · Jul 19, 2020 · 402529a · 402529a
1 parent f406cc5
commit 402529a
Show file tree

Hide file tree

Showing 10 changed files with 24 additions and 234 deletions.
diff --git a/GitHubConfiguration.ps1 b/GitHubConfiguration.ps1
@@ -147,6 +147,11 @@ function Set-GitHubConfiguration
     .PARAMETER RetryDelaySeconds
         The number of seconds to wait before retrying a command again after receiving a 202 response.
 
+    .PARAMETER StateChangeDelaySeconds
+        The number of seconds to wait before returning the result after executing a command that
+        may result in a state change on the server.  This is intended to only be used during test
+        execution in order to increase reliability.
+
     .PARAMETER SuppressNoTokenWarning
         If an Access Token has not been configured, this module will provide a warning to the user
         informing them of this, once per session.  If it is expected that this module will regularly
@@ -228,6 +233,8 @@ function Set-GitHubConfiguration
 
         [int] $RetryDelaySeconds,
 
+        [int] $StateChangeDelaySeconds,
+
         [switch] $SuppressNoTokenWarning,
 
         [switch] $SuppressTelemetryReminder,
@@ -312,6 +319,7 @@ function Get-GitHubConfiguration
             'LogTimeAsUtc',
             'MultiRequestProgressThreshold',
             'RetryDelaySeconds',
+            'StateChangeDelaySeconds',
             'SuppressNoTokenWarning',
             'SuppressTelemetryReminder',
             'TestConfigSettingsHash',
@@ -659,6 +667,7 @@ function Import-GitHubConfiguration
         'logTimeAsUtc' = $false
         'multiRequestProgressThreshold' = 10
         'retryDelaySeconds' = 30
+        'stateChangeDelaySeconds' = 0
         'suppressNoTokenWarning' = $false
         'suppressTelemetryReminder' = $false
         'webRequestTimeoutSec' = 0

diff --git a/GitHubCore.ps1 b/GitHubCore.ps1
@@ -321,6 +321,16 @@ function Invoke-GHRestMethod
             }
         }
 
+        # Allow for a delay after a command that may result in a state change in order to
+        #increase the reliability of the UT's which attempt multiple successive state change
+        # on the same object.
+        $stateChangeDelaySeconds = $(Get-GitHubConfiguration -Name 'StateChangeDelaySeconds')
+        $stateChangeMethods = @('Delete', 'Post', 'Patch', 'Put')
+        if (($stateChangeDelaySeconds -gt 0) -and ($Method -in $stateChangeMethods))
+        {
+            Start-Sleep -Seconds $stateChangeDelaySeconds
+        }
+
         if ($ExtendedResult)
         {
             $finalResultEx = @{

diff --git a/Tests/Common.ps1 b/Tests/Common.ps1
@@ -101,9 +101,10 @@ function Initialize-CommonTestSetup
     Set-GitHubConfiguration -MultiRequestProgressThreshold 0 # Status corrupts the raw CI logs for Linux and Mac, and makes runs take slightly longer.
     Set-GitHubConfiguration -DisableUpdateCheck # The update check is unnecessary during tests.
 
-    # The number of seconds to sleep after performing some operations to ensure that successive
-    # API calls properly reflect previously updated state.
-    $script:defaultSleepSecondsForReliability = 3
+    # We execute so many successive state changing commands on the same object that sometimes
+    # GitHub gets confused.  We'll add an intentional delay to slow down our execution in an effort
+    # to increase the reliability of the tests.
+    Set-GitHubConfiguration -StateChangeDelaySeconds 3
 }
 
 Initialize-CommonTestSetup
diff --git a/Tests/GitHubAssignees.tests.ps1 b/Tests/GitHubAssignees.tests.ps1
@@ -108,21 +108,13 @@ try
         Context 'Adding and removing an assignee via parameters' {
             $issue = $repo | New-GitHubIssue -Title "Test issue"
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have no assignees when created' {
                 $issue.assignee.login | Should -BeNullOrEmpty
                 $issue.assignees | Should -BeNullOrEmpty
             }
 
             $updatedIssue = Add-GitHubAssignee -OwnerName $script:ownerName -RepositoryName $repo.name -Issue $issue.number -Assignee $owner.login
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have returned the same issue' {
                 $updatedIssue.number | Should -Be $issue.number
             }
@@ -155,21 +147,13 @@ try
         Context 'Adding an assignee with the repo on the pipeline' {
             $issue = $repo | New-GitHubIssue -Title "Test issue"
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have no assignees when created' {
                 $issue.assignee.login | Should -BeNullOrEmpty
                 $issue.assignees | Should -BeNullOrEmpty
             }
 
             $updatedIssue = $repo | Add-GitHubAssignee -Issue $issue.number -Assignee $owner.login
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have returned the same issue' {
                 $updatedIssue.number | Should -Be $issue.number
             }
@@ -202,21 +186,13 @@ try
         Context 'Adding an assignee with the issue on the pipeline' {
             $issue = $repo | New-GitHubIssue -Title "Test issue"
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have no assignees when created' {
                 $issue.assignee.login | Should -BeNullOrEmpty
                 $issue.assignees | Should -BeNullOrEmpty
             }
 
             $updatedIssue = $issue | Add-GitHubAssignee -Assignee $owner.login
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have returned the same issue' {
                 $updatedIssue.number | Should -Be $issue.number
             }
@@ -249,21 +225,13 @@ try
         Context 'Adding an assignee with the assignee user object on the pipeline' {
             $issue = $repo | New-GitHubIssue -Title "Test issue"
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have no assignees when created' {
                 $issue.assignee.login | Should -BeNullOrEmpty
                 $issue.assignees | Should -BeNullOrEmpty
             }
 
             $updatedIssue = $owner | Add-GitHubAssignee -OwnerName $script:ownerName -RepositoryName $repo.name -Issue $issue.number
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             It 'Should have returned the same issue' {
                 $updatedIssue.number | Should -Be $issue.number
             }

diff --git a/Tests/GitHubContents.tests.ps1 b/Tests/GitHubContents.tests.ps1
@@ -37,10 +37,6 @@ try
         BeforeAll {
             # AutoInit will create a readme with the GUID of the repo name
             $repo = New-GitHubRepository -RepositoryName ($repoGuid) -AutoInit
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
         }
 
         AfterAll {
@@ -262,12 +258,7 @@ try
     Describe 'GitHubContents/Set-GitHubContent' {
         BeforeAll {
             $repoName = [Guid]::NewGuid().Guid
-
             $repo = New-GitHubRepository -RepositoryName $repoName -AutoInit
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
         }
 
         Context 'When setting new file content' {

diff --git a/Tests/GitHubIssues.tests.ps1 b/Tests/GitHubIssues.tests.ps1
@@ -161,9 +161,6 @@ try
             for ($i = 0; $i -lt 4; $i++)
             {
                 $newIssues += New-GitHubIssue -OwnerName $script:ownerName -RepositoryName $repo.name -Title ([Guid]::NewGuid().Guid)
-
-                 # Needed to ensure that there is a unique creation timestamp between issues
-                Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
             }
 
             $newIssues[0] = Set-GitHubIssue -OwnerName $script:ownerName -RepositoryName $repo.name -Issue $newIssues[0].number -State Closed
@@ -550,10 +547,6 @@ try
 
             Lock-GitHubIssue -OwnerName $script:OwnerName -RepositoryName $repo.name -Issue $issue.number
 
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $timeline = @(Get-GitHubIssueTimeline -OwnerName $script:OwnerName -RepositoryName $repo.name -Issue $issue.number)
             It 'Should have an event now' {
                 $timeline.Count | Should -Be 1

diff --git a/Tests/GitHubLabels.tests.ps1 b/Tests/GitHubLabels.tests.ps1
@@ -80,11 +80,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             Initialize-GitHubLabel -OwnerName $script:ownerName -RepositoryName $repositoryName -Label $defaultLabels
         }
 
@@ -209,10 +204,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
         }
 
         AfterAll {
@@ -327,10 +318,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
         }
 
         AfterAll {
@@ -339,11 +326,6 @@ try
 
         Context 'Removing a label with parameters' {
             $label = $repo | New-GitHubLabel -Label 'test' -Color 'CCCCCC'
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             Remove-GitHubLabel -OwnerName $script:ownerName -RepositoryName $repositoryName -Label $label.name -Force
 
             It 'Should be gone after being removed by parameter' {
@@ -353,11 +335,6 @@ try
 
         Context 'Removing a label with the repo on the pipeline' {
             $label = $repo | New-GitHubLabel -Label 'test' -Color 'CCCCCC'
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $repo | Remove-GitHubLabel -Label $label.name -Confirm:$false
 
             It 'Should be gone after being removed by parameter' {
@@ -367,11 +344,6 @@ try
 
         Context 'Removing a label with the name on the pipeline' {
             $label = $repo | New-GitHubLabel -Label 'test' -Color 'CCCCCC'
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $label.name | Remove-GitHubLabel -OwnerName $script:ownerName -RepositoryName $repositoryName -Force
 
             It 'Should be gone after being removed by parameter' {
@@ -381,11 +353,6 @@ try
 
         Context 'Removing a label with the label object on the pipeline' {
             $label = $repo | New-GitHubLabel -Label 'test' -Color 'CCCCCC'
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $label | Remove-GitHubLabel -Force
 
             It 'Should be gone after being removed by parameter' {
@@ -398,10 +365,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
         }
 
         AfterAll {
@@ -579,10 +542,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
         }
 
         AfterAll {
@@ -657,11 +616,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $repo | Initialize-GitHubLabel -Label $defaultLabels
         }
 
@@ -890,11 +844,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $repo | Initialize-GitHubLabel -Label $defaultLabels
         }
 
@@ -961,11 +910,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $repo | Initialize-GitHubLabel -Label $defaultLabels
         }
 
@@ -1133,11 +1077,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $repo | Initialize-GitHubLabel -Label $defaultLabels
         }
 
@@ -1287,11 +1226,6 @@ try
         BeforeAll {
             $repositoryName = [Guid]::NewGuid().Guid
             $repo = New-GitHubRepository -RepositoryName $repositoryName
-
-            # The CI build has been unreliable with this test.
-            # Adding a short sleep to ensure successive queries reflect updated state.
-            Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
             $repo | Initialize-GitHubLabel -Label $defaultLabels
 
             $milestone = $repo | New-GitHubMilestone -Title 'test milestone'

diff --git a/Tests/GitHubProjects.tests.ps1 b/Tests/GitHubProjects.tests.ps1
@@ -611,10 +611,6 @@ try
             $project = New-GitHubProject -OwnerName $script:ownerName -RepositoryName $repo.name -ProjectName $defaultRepoProject -Description $defaultRepoProjectDesc
             $null = Remove-GitHubProject -Project $project.id -Confirm:$false
             It 'Project should be removed' {
-                # The CI build has been unreliable with this test.
-                # Adding a short sleep to ensure successive queries reflect updated state.
-                Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
                 {Get-GitHubProject -Project $project.id} | Should -Throw
             }
         }
@@ -623,10 +619,6 @@ try
             $project = $repo | New-GitHubProject -ProjectName $defaultRepoProject -Description $defaultRepoProjectDesc
             $project | Remove-GitHubProject -Force
             It 'Project should be removed' {
-                # The CI build has been unreliable with this test.
-                # Adding a short sleep to ensure successive queries reflect updated state.
-                Start-Sleep -Seconds $script:defaultSleepSecondsForReliability
-
                 {$project | Get-GitHubProject} | Should -Throw
             }
         }