From 731247876d4f38a609af34eacfccfdf33fbe07b6 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Tue, 13 Mar 2018 21:26:55 +0800 Subject: [PATCH] [Pending] Finally we add the code linting and its tests! (#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 --- .gitignore | 1 + .vscode/settings.json | 4 ++++ PSScriptAnalyzerSettings.psd1 | 37 +++++++++++++++++++++++++++++++ appveyor.yml | 2 ++ bin/checkurls.ps1 | 2 +- bin/install.ps1 | 4 ++-- lib/autoupdate.ps1 | 6 ++--- lib/config.ps1 | 4 ++-- lib/core.ps1 | 4 ++-- lib/description.ps1 | 4 +++- lib/install.ps1 | 2 +- lib/manifest.ps1 | 4 ++-- lib/versions.ps1 | 2 +- libexec/scoop-cleanup.ps1 | 2 +- libexec/scoop-virustotal.ps1 | 14 ++++++------ libexec/scoop-which.ps1 | 7 ++++-- test/00-Project.Tests.ps1 | 12 ++++++++++ test/Scoop-Linting.Tests.ps1 | 41 +++++++++++++++++++++++++++++++++++ test/Scoop-Manifest.Tests.ps1 | 6 ++--- test/Scoop-TestLib.ps1 | 2 +- 20 files changed, 131 insertions(+), 29 deletions(-) create mode 100644 .vscode/settings.json create mode 100644 PSScriptAnalyzerSettings.psd1 create mode 100644 test/Scoop-Linting.Tests.ps1 diff --git a/.gitignore b/.gitignore index 71f9152c12..cde3143c76 100644 --- 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 new file mode 100644 index 0000000000..6be0713a88 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,4 @@ +// Configure PSScriptAnalyzer settings +{ + "powershell.scriptAnalysis.settingsPath": "PSScriptAnalyzerSettings.psd1" +} diff --git a/PSScriptAnalyzerSettings.psd1 b/PSScriptAnalyzerSettings.psd1 new file mode 100644 index 0000000000..4e6ee2f57f --- /dev/null +++ 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 index 744530d93e..88e130e9e9 100644 --- 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 index 5b4bf71506..e6de8c9964 100644 --- 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 index c8bb8dec52..1007b4e46b 100644 --- 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 index 6052ae1bf9..5e4be2c28e 100644 --- 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 index 1bad35091a..50483b0a56 100644 --- 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 index cfb66df4aa..555870fc20 100644 --- 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 index 1ca015f8a2..4beab24ad9 100644 --- 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 index b32f11c7e4..da3d33de5c 100644 --- 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 index 02a2f7c377..42acbe10bb 100644 --- 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 index fa16a79dac..d0c5d4d782 100644 --- 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 index 0053ab0a25..603031f029 100644 --- 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 index 01b5c954c3..b671cda4ed 100644 --- 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,11 +70,11 @@ Function Navigate-ToHash($hash, $app) { return 0 } -Function Start-VirusTotal ($h, $app) { +Function Search-VirusTotal ($h, $app) { if ($h -match "(?[^:]+):(?.*)") { $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.") @@ -82,7 +82,7 @@ Function Start-VirusTotal ($h, $app) { } } 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 index e395ac92c4..2f477c61bb 100644 --- a/libexec/scoop-which.ps1 +++ b/libexec/scoop-which.ps1 @@ -9,8 +9,11 @@ reset_aliases if(!$command) { 'ERROR: 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 index 514137dfdc..e03d8cc131 100644 --- 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 new file mode 100644 index 0000000000..b209f93f07 --- /dev/null +++ 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 index 230736bf03..131980e7c0 100644 --- 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 index e72d72e9b6..88332e5a92 100644 --- 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 }