[Pending] Finally we add the code linting and its tests! (ScoopInstal…

…ler#2108) * Lint: PSPossibleIncorrectComparisonWithNull * Lint: PSUseLiteralInitializerForHashtable * Lint: PSUseBOMForUnicodeEncodedFile * Lint: PSUseApprovedVerbs * Lint: PSAvoidGlobalVars * Lint: PSAvoidUsingEmptyCatchBlock * Lint: PSUseShouldProcessForStateChangingFunctions * Lint helper: Add PSScriptAnalyzer integration for vscode * Fix lint: PSUseBOMForUnicodeEncodedFile * Tests: ignore previous TestResults.xml * Tests: add PowerShell script linting into tests! * Add PSScriptAnalyzer into appveyor ci * Update Scoop-Linting.Tests.ps1
Josantoz · Mar 13, 2018 · 7312478 · 7312478
1 parent 2d9f964
commit 7312478
Show file tree

Hide file tree

Showing 20 changed files with 131 additions and 29 deletions.
diff --git a/.gitignore b/.gitignore
@@ -4,3 +4,4 @@ scoop.sublime-workspace
 test/installer/tmp/*
 test/tmp/*
 *~
+TestResults.xml
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -0,0 +1,4 @@
+// Configure PSScriptAnalyzer settings
+{
+    "powershell.scriptAnalysis.settingsPath": "PSScriptAnalyzerSettings.psd1"
+}
diff --git a/PSScriptAnalyzerSettings.psd1 b/PSScriptAnalyzerSettings.psd1
@@ -0,0 +1,37 @@
+# The PowerShell Script Analyzer will generate a warning
+# diagnostic record for this file due to a bug -
+# https://github.com/PowerShell/PSScriptAnalyzer/issues/472
+@{
+    # Only diagnostic records of the specified severity will be generated.
+    # Uncomment the following line if you only want Errors and Warnings but
+    # not Information diagnostic records.
+    Severity = @('Error','Warning')
+
+    # Analyze **only** the following rules. Use IncludeRules when you want
+    # to invoke only a small subset of the defualt rules.
+    # IncludeRules = @('PSAvoidDefaultValueSwitchParameter',
+    #                  'PSMisleadingBacktick',
+    #                  'PSMissingModuleManifestField',
+    #                  'PSReservedCmdletChar',
+    #                  'PSReservedParams',
+    #                  'PSShouldProcess',
+    #                  'PSUseApprovedVerbs',
+    #                  'PSAvoidUsingCmdletAliases',
+    #                  'PSUseDeclaredVarsMoreThanAssignments')
+
+    # Do not analyze the following rules. Use ExcludeRules when you have
+    # commented out the IncludeRules settings above and want to include all
+    # the default rules except for those you exclude below.
+    # Note: if a rule is in both IncludeRules and ExcludeRules, the rule
+    # will be excluded.
+    ExcludeRules = @(
+        # Currently Scoop widely uses Write-Host to output colored text.
+        'PSAvoidUsingWriteHost',
+        # Temporarily allow uses of Invoke-Expression,
+        # this command is used by some core functions and hard to be removed.
+        'PSAvoidUsingInvokeExpression',
+        # PSUseDeclaredVarsMoreThanAssignments doesn't currently work due to:
+        # https://github.com/PowerShell/PSScriptAnalyzer/issues/636
+        'PSUseDeclaredVarsMoreThanAssignments'
+    )
+}
diff --git a/appveyor.yml b/appveyor.yml
@@ -6,7 +6,9 @@ branches:
 
 init:
   - ps: (get-psprovider 'FileSystem').Home = $(pwd)
+  - ps: Install-PackageProvider Nuget -Force
   - ps: Install-Module -Name Pester -Repository PSGallery -Force
+  - ps: Install-Module -Name PSScriptAnalyzer -Repository PSGallery -Force
 
 build: off
 

diff --git a/bin/checkurls.ps1 b/bin/checkurls.ps1
@@ -58,7 +58,7 @@ function test_dl($url, $cookies) {
         if($e.innerexception) { $e = $e.innerexception }
         return $url, "Error", $e.message
     } finally {
-        if($wres -ne $null -and $wres -isnot [net.ftpwebresponse]) {
+        if($null -ne $wres -and $wres -isnot [net.ftpwebresponse]) {
             $wres.close()
         }
     }

diff --git a/bin/install.ps1 b/bin/install.ps1
@@ -1,4 +1,4 @@
-#requires -v 3
+# requires -v 3
 
 # remote install:
 #   iex (new-object net.webclient).downloadstring('https://get.scoop.sh')
@@ -26,7 +26,7 @@ Invoke-Expression (new-object net.webclient).downloadstring($core_url)
 # prep
 if(installed 'scoop') {
     write-host "Scoop is already installed. Run 'scoop update' to get the latest version." -f red
-    # don't abort if invoked with iex——that would close the PS session
+    # don't abort if invoked with iex that would close the PS session
     if($myinvocation.mycommand.commandtype -eq 'Script') { return } else { exit 1 }
 }
 $dir = ensure (versiondir 'scoop' 'current')

diff --git a/lib/autoupdate.ps1 b/lib/autoupdate.ps1
@@ -167,7 +167,7 @@ function get_hash_for_app([String] $app, $config, [String] $version, [String] $u
 function update_manifest_with_new_version($json, [String] $version, [String] $url, [String] $hash, $architecture = $null) {
     $json.version = $version
 
-    if ($architecture -eq $null) {
+    if ($null -eq $architecture) {
         if ($json.url -is [System.Array]) {
             $json.url[0] = $url
             $json.hash[0] = $hash
@@ -247,7 +247,7 @@ function autoupdate([String] $app, $dir, $json, [String] $version, [Hashtable] $
         if($valid) {
             # create hash
             $hash = get_hash_for_app $app $json.autoupdate.hash $version $url $substitutions
-            if ($hash -eq $null) {
+            if ($null -eq $hash) {
                 $valid = $false
                 Write-Host -f DarkRed "Could not find hash!"
             }
@@ -273,7 +273,7 @@ function autoupdate([String] $app, $dir, $json, [String] $version, [Hashtable] $
             if($valid) {
                 # create hash
                 $hash = get_hash_for_app $app (arch_specific "hash" $json.autoupdate $architecture) $version $url $substitutions
-                if ($hash -eq $null) {
+                if ($null -eq $hash) {
                     $valid = $false
                     Write-Host -f DarkRed "Could not find hash!"
                 }

diff --git a/lib/config.ps1 b/lib/config.ps1
@@ -9,7 +9,7 @@ function hashtable($obj) {
 }
 
 function hashtable_val($obj) {
-    if($obj -eq $null) { return $null }
+    if($null -eq $obj) { return $null }
     if($obj -is [array]) {
         $arr = @()
         $obj | ForEach-Object {
@@ -49,7 +49,7 @@ function set_config($name, $val) {
         $cfg.$name = $val
     }
 
-    if($val -eq $null) {
+    if($null -eq $val) {
         $cfg.remove($name)
     }
 

diff --git a/lib/core.ps1 b/lib/core.ps1
@@ -78,7 +78,7 @@ function cache_path($app, $version, $url) { "$cachedir\$app#$version#$($url -rep
 # apps
 function sanitary_path($path) { return [regex]::replace($path, "[/\\?:*<>|]", "") }
 function installed($app, $global=$null) {
-    if($global -eq $null) { return (installed $app $true) -or (installed $app $false) }
+    if($null -eq $global) { return (installed $app $true) -or (installed $app $false) }
     return is_directory (appdir $app $global)
 }
 function installed_apps($global) {
@@ -352,7 +352,7 @@ function ensure_all_installed($apps, $global) {
 }
 
 function strip_path($orig_path, $dir) {
-    if($orig_path -eq $null) { $orig_path = '' }
+    if($null -eq $orig_path) { $orig_path = '' }
     $stripped = [string]::join(';', @( $orig_path.split(';') | Where-Object { $_ -and $_ -ne $dir } ))
     return ($stripped -ne $orig_path), $stripped
 }

diff --git a/lib/description.ps1 b/lib/description.ps1
@@ -107,7 +107,9 @@ function strip_html($html) {
             $charset = $matches[1]
             try {
                 $encoding = [text.encoding]::getencoding($charset)
-            } catch { }
+            } catch {
+                Write-Warning "Unknown charset"
+            }
             if($encoding) {
                 $html = ([regex]'&#(\d+);?').replace($html, {
                     param($m)

diff --git a/lib/install.ps1 b/lib/install.ps1
@@ -109,7 +109,7 @@ function dl_with_cache($app, $version, $url, $to, $cookies = $null, $use_cache =
         Move-Item "$cached.download" $cached -force
     } else { write-host "Loading $(url_remote_filename $url) from cache"}
 
-    if (!($to -eq $null)) {
+    if (!($null -eq $to)) {
         Copy-Item $cached $to
     }
 }

diff --git a/lib/manifest.ps1 b/lib/manifest.ps1
@@ -38,7 +38,7 @@ function installed_manifest($app, $version, $global) {
 }
 
 function save_install_info($info, $dir) {
-    $nulls = $info.keys | Where-Object { $info[$_] -eq $null }
+    $nulls = $info.keys | Where-Object { $null -eq $info[$_] }
     $nulls | ForEach-Object { $info.remove($_) } # strip null-valued
 
     $info | convertto-json | out-file "$dir\install.json"
@@ -83,7 +83,7 @@ function generate_user_manifest($app, $version) {
 
     ensure $(usermanifestsdir) | out-null
     try {
-        autoupdate $app "$(resolve-path $(usermanifestsdir))" $manifest $version $(New-Object HashTable)
+        autoupdate $app "$(resolve-path $(usermanifestsdir))" $manifest $version $(@{})
         return "$(resolve-path $(usermanifest $app))"
     } catch {
         write-host -f darkred "Could not install $app@$version"

diff --git a/lib/versions.ps1 b/lib/versions.ps1
@@ -38,7 +38,7 @@ function compare_versions($a, $b) {
 }
 
 function qsort($ary, $fn) {
-    if($ary -eq $null) { return @() }
+    if($null -eq $ary) { return @() }
     if(!($ary -is [array])) { return @($ary) }
 
     $pivot = $ary[0]

diff --git a/libexec/scoop-cleanup.ps1 b/libexec/scoop-cleanup.ps1
@@ -41,7 +41,7 @@ function cleanup($app, $global, $verbose) {
         $dir = versiondir $app $version $global
         Get-ChildItem $dir | ForEach-Object {
             $file = $_
-            if($file.LinkType -ne $null) {
+            if($null -ne $file.LinkType) {
                 fsutil.exe reparsepoint delete $file.FullName | out-null
             }
         }

diff --git a/libexec/scoop-virustotal.ps1 b/libexec/scoop-virustotal.ps1
@@ -43,7 +43,7 @@ $_ERR_NO_INFO = 8
 
 $exit_code = 0
 
-Function Navigate-ToHash($hash, $app) {
+Function Get-VirusTotalResult($hash, $app) {
     $hash = $hash.ToLower()
     $url = "https://www.virustotal.com/ui/files/$hash"
     $api_key = get_config("virustotal_api_key")
@@ -70,19 +70,19 @@ Function Navigate-ToHash($hash, $app) {
     return 0
 }
 
-Function Start-VirusTotal ($h, $app) {
+Function Search-VirusTotal ($h, $app) {
     if ($h -match "(?<algo>[^:]+):(?<hash>.*)") {
         $hash = $matches["hash"]
         if ($matches["algo"] -match "(md5|sha1|sha256)") {
-            return Navigate-ToHash $hash $app
+            return Get-VirusTotalResult $hash $app
         }
         else {
             warn("$app`: Unsupported hash $($matches['algo']). VirusTotal needs md5, sha1 or sha256.")
             return $_ERR_NO_INFO
         }
     }
     else {
-        return Navigate-ToHash $h $app
+        return Get-VirusTotalResult $h $app
     }
 }
 
@@ -109,7 +109,7 @@ Function Get-RedirectedUrl {
     return $redir
 }
 
-Function SubmitMaybe-ToVirusTotal ($url, $app, $do_scan) {
+Function Submit-ToVirusTotal ($url, $app, $do_scan) {
     if ($do_scan) {
         try {
             # Follow redirections (for e.g. sourceforge URLs) because
@@ -198,11 +198,11 @@ $apps | ForEach-Object {
             Start-Sleep -s (50 + ($requests * 2))
         }
         try {
-            $exit_code = $exit_code -bor (Start-VirusTotal $_ $app)
+            $exit_code = $exit_code -bor (Search-VirusTotal $_ $app)
         } catch [Exception] {
             $exit_code = $exit_code -bor $_ERR_EXCEPTION
             if ($_.Exception.Message -like "*(404)*") {
-                SubmitMaybe-ToVirusTotal $url[$i] $app ($opt.scan -or $opt.s)
+                Submit-ToVirusTotal $url[$i] $app ($opt.scan -or $opt.s)
             }
             else {
                 if ($_.Exception.Message -match "\(204|429\)") {

diff --git a/libexec/scoop-which.ps1 b/libexec/scoop-which.ps1
@@ -9,8 +9,11 @@ reset_aliases
 
 if(!$command) { 'ERROR: <command> missing'; my_usage; exit 1 }
 
-try { $gcm = Get-Command "$command" -ea stop } catch { } #
-if(!$gcm) { [console]::error.writeline("'$command' not found"); exit 3 }
+try {
+    $gcm = Get-Command "$command" -ea stop
+} catch {
+    [console]::error.writeline("'$command' not found"); exit 3
+}
 
 $path = "$($gcm.path)"
 $usershims = "$(resolve-path $(shimdir $false))"

diff --git a/test/00-Project.Tests.ps1 b/test/00-Project.Tests.ps1
@@ -100,6 +100,10 @@ describe 'Style constraints for non-binary project files' {
         $badFiles = @(
             foreach ($file in $files)
             {
+                # Ignore previous TestResults.xml
+                if ($file -match "TestResults.xml") {
+                    continue
+                }
                 $content = ([char[]](Get-Content $file.FullName -encoding byte -totalcount 3) -join '')
                 if ([regex]::match($content, '(?ms)^\xEF\xBB\xBF').success)
                 {
@@ -118,6 +122,10 @@ describe 'Style constraints for non-binary project files' {
         $badFiles = @(
             foreach ($file in $files)
             {
+                # Ignore previous TestResults.xml
+                if ($file -match "TestResults.xml") {
+                    continue
+                }
                 $string = [System.IO.File]::ReadAllText($file.FullName)
                 if ($string.Length -gt 0 -and $string[-1] -ne "`n")
                 {
@@ -164,6 +172,10 @@ describe 'Style constraints for non-binary project files' {
         $badLines = @(
             foreach ($file in $files)
             {
+                # Ignore previous TestResults.xml
+                if ($file -match "TestResults.xml") {
+                    continue
+                }
                 $lines = [System.IO.File]::ReadAllLines($file.FullName)
                 $lineCount = $lines.Count
 

diff --git a/test/Scoop-Linting.Tests.ps1 b/test/Scoop-Linting.Tests.ps1
@@ -0,0 +1,41 @@
+$repo_dir = (Get-Item $MyInvocation.MyCommand.Path).directory.parent.FullName
+
+Describe "PSScriptAnalyzer" {
+    $scoop_modules = Get-ChildItem $repo_dir -Recurse -Include *.psd1, *.psm1, *.ps1
+    $scoop_modules = $scoop_modules | Where-Object { $_.DirectoryName -notlike '*\supporting*' -and $_.DirectoryName -notlike '*\test*' }
+    $scoop_modules = $scoop_modules | Select-Object -ExpandProperty Directory -Unique
+
+    Context "Checking ScriptAnalyzer" {
+        It "Invoke-ScriptAnalyzer Cmdlet should exist" {
+            { Get-Command Invoke-ScriptAnalyzer -ErrorAction Stop } | Should Not Throw
+        }
+        It "PSScriptAnalyzerSettings.ps1 should exist" {
+            Test-Path "$repo_dir\PSScriptAnalyzerSettings.psd1" | Should Be $true
+        }
+        It "There should be files to test" {
+            $scoop_modules.Count | Should Not Be 0
+        }
+    }
+
+    $linting_settings = Get-Item -Path "$repo_dir\PSScriptAnalyzerSettings.psd1"
+
+    Context "Linting all *.psd1, *.psm1 and *.ps1 files" {
+        foreach($directory in $scoop_modules) {
+            $analysis = Invoke-ScriptAnalyzer -Path $directory.FullName -Settings $linting_settings.FullName
+            It "Should pass: $directory" {
+                $analysis.Count | Should Be 0
+            }
+            if($analysis) {
+                foreach($result in $analysis) {
+                    switch -wildCard ($result.ScriptName) {
+                        '*.psm1' { $type = 'Module' }
+                        '*.ps1'  { $type = 'Script' }
+                        '*.psd1' { $type = 'Manifest' }
+                    }
+                    Write-Host -f Yellow "      [*] $($result.Severity): $($result.Message)"
+                    Write-Host -f Yellow "          $($result.RuleName) in $type`: $directory\$($result.ScriptName):$($result.Line)"
+                }
+            }
+        }
+    }
+}
diff --git a/test/Scoop-Manifest.Tests.ps1 b/test/Scoop-Manifest.Tests.ps1
@@ -50,13 +50,13 @@ describe "manifest-validation" {
             $validator = new-object Scoop.Validator($schema, $true)
         }
 
-        $global:quota_exceeded = $false
+        $quota_exceeded = $false
 
         $manifest_files | ForEach-Object {
             it "$_" {
                 $file = $_ # exception handling may overwrite $_
 
-                if(!($global:quota_exceeded)) {
+                if(!($quota_exceeded)) {
                     try {
                         $validator.Validate($file.fullname)
 
@@ -66,7 +66,7 @@ describe "manifest-validation" {
                         $validator.Errors.Count | should be 0
                     } catch {
                         if($_.exception.message -like '*The free-quota limit of 1000 schema validations per hour has been reached.*') {
-                            $global:quota_exceeded = $true
+                            $quota_exceeded = $true
                             write-host -f darkyellow 'Schema validation limit exceeded. Will skip further validations.'
                         } else {
                             throw

diff --git a/test/Scoop-TestLib.ps1 b/test/Scoop-TestLib.ps1
@@ -59,7 +59,7 @@ function script:fail($msg) {
 }
 
 function script:fmt($var) {
-    if($var -eq $null) { return "`$null" }
+    if($null -eq $var) { return "`$null" }
     if($var -is [string]) { return "'$var'" }
     return $var
 }