Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows: Build and use gotestsum for running all tests #39998

Merged
merged 1 commit into from
Dec 23, 2019
Merged

Windows: Build and use gotestsum for running all tests #39998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 40 additions & 5 deletions Dockerfile.windows
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ FROM microsoft/windowsservercore
SHELL ["powershell", "-Command", "$ErrorActionPreference = 'Stop'; $ProgressPreference = 'SilentlyContinue';"]

ARG GO_VERSION=1.13.4
ARG GOTESTSUM_COMMIT=v0.3.5

# Environment variable notes:
# - GO_VERSION must be consistent with 'Dockerfile' used by Linux.
Expand All @@ -174,7 +175,8 @@ ENV GO_VERSION=${GO_VERSION} `
GIT_VERSION=2.11.1 `
GOPATH=C:\gopath `
GO111MODULE=off `
FROM_DOCKERFILE=1
FROM_DOCKERFILE=1 `
GOTESTSUM_COMMIT=${GOTESTSUM_COMMIT}

RUN `
Function Test-Nano() { `
Expand Down Expand Up @@ -249,13 +251,46 @@ RUN `
Remove-Item C:\binutils.zip; `
Remove-Item C:\gitsetup.zip; `
`
Write-Host INFO: Creating source directory...; `
New-Item -ItemType Directory -Path ${GOPATH}\src\github.com\docker\docker | Out-Null; `
# Ensure all directories exist that we will require below....
$srcDir = """$Env:GOPATH`\src\github.com\docker\docker\bundles"""; `
Write-Host INFO: Ensuring existence of directory $srcDir...; `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does running the command below itself show up in the logs? If so, this might be redundant

Copy link

@vikramhh vikramhh Nov 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is primarily for consistency with the code around this line. Dockerfile.Windows uses this patttern consistently [look at lines above and below this piece of code]. Just to make sure though, I also verified that it is not redundant and that the command does not show up in the logs by itself.

New-Item -Force -ItemType Directory -Path $srcDir | Out-Null; `
`
Write-Host INFO: Configuring git core.autocrlf...; `
C:\git\cmd\git config --global core.autocrlf true; `
C:\git\cmd\git config --global core.autocrlf true;

RUN `
Function Build-GoTestSum() { `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scripts that we're writing in the Dockerfile now are a bit lengthy, and more complex to maintain (having to keep all the line-continuations (```) into account.

Perhaps we should put these function in a .ps file (easier to write/maintain), and;

Somewhere at the top of the Dockerfile (assuming the functions themself don't change often);

COPY hack/dockerfile/install/gotestsum.installer.ps1 hack/dockerfile/install/gotestsum.installer.ps1

And later on, call those functions

RUN . "hack/dockerfile/install/gotestsum.installer.ps1" Build-GoTestSum

(or however that should be done in PowerShell)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion and something I have put on my plate. It will be done in a separate PR

Write-Host "INFO: Building gotestsum version $Env:GOTESTSUM_COMMIT in $Env:GOPATH"; `
$optsForGet = @('"get"', '"-d"', '"gotest.tools/gotestsum"'); `
&go $optsForGet; `
$savedExitCode = $LASTEXITCODE; `
if ($savedExitCode -ne 0) { `
Throw '"Failed getting gotestsum sources..."' `
}; `
Write-Host "INFO: Sources obtained for gotestsum..."; `
$GotestsumPath=Join-Path -Path $Env:GOPATH -ChildPath "src\gotest.tools\gotestsum"; `
Push-Location $GotestsumPath; `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like $GotestsumPath is only used here, so better to move declaration of that variable also here

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

$optsForCheckout = @('"checkout"', '"-q"', """$Env:GOTESTSUM_COMMIT"""); `
&git $optsForCheckout; `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still in favour of inlining these (I'll write up what I think it could look like below)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this should not be a blocker for the PR.

$savedExitCode = $LASTEXITCODE; `
if ($savedExitCode -eq 0) { `
Copy link
Member Author

@thaJeztah thaJeztah Nov 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you reverse the logic here, we can Throw directly here, and probably don't need the $savedExitCode. With the changes I suggested, it would look like;

  Function Build-GoTestSum() { `
    Write-Host "INFO: Building gotestsum version $Env:GOTESTSUM_COMMIT"; `
    &go get -d gotest.tools/gotestsum; `
    if ($LASTEXITCODE -ne 0) {  `
      Throw '"Failed getting gotestsum sources..."'  `
    }; `
    Write-Host "INFO:     Sources obtained for gotestsum..."; `
    $GotestsumPath=Join-Path -Path $Env:GOPATH -ChildPath "src\gotest.tools\gotestsum"; `
    Push-Location $GotestsumPath; `
    &git checkout -q "$Env:GOTESTSUM_COMMIT"; `
    if ($LASTEXITCODE -ne 0) { `
      Throw '"gotestsum checkout failed..."'; `
    } `
    Write-Host "INFO:     Checkout done for gotestsum..."; `
    &go build --buildmode=exe; `
    if ($LASTEXITCODE -ne 0) {  `
      Throw '"gotestsum build failed..."'; `
    } `
    Pop-Location; `
    Write-Host "INFO:     Build done for gotestsum..."; `
  } `
  `
  Build-GoTestSum

Looking further, perhaps if we change ErrorActionPreference

$ErrorActionPreference = "stop"

edit: actually; that's already set in the dockerfile:

moby/Dockerfile.windows

Line 166 in 6355842

SHELL ["powershell", "-Command", "$ErrorActionPreference = 'Stop'; $ProgressPreference = 'SilentlyContinue';"]

In which case, we may not even need the manual handling of LASTEXITCODE. If I'm right, we could simplify the script quite a lot. Taking if further; given that everything is in a single run, we wouldn't have to worry about Push-location / Pop-location, and just change to the $GotestsumPath directory. At that point, the function is so minimal, I'd even consider just not creating a function for it;

  Write-Host "INFO: Building gotestsum version $Env:GOTESTSUM_COMMIT"; `
  &go get -d gotest.tools/gotestsum; `
  Write-Host "INFO:     Sources obtained for gotestsum..."; `
  $GotestsumPath=Join-Path -Path $Env:GOPATH -ChildPath "src\gotest.tools\gotestsum"; `
  Set-Location -Path $GotestsumPath; `
  &git checkout -q "$Env:GOTESTSUM_COMMIT"; `
  Write-Host "INFO:     Checkout done for gotestsum..."; `
  &go build --buildmode=exe; `
  Write-Host "INFO:     Build done for gotestsum...";

(even the INFO logs may be a bit redundant at that point);

  Write-Host "INFO: Building gotestsum version $Env:GOTESTSUM_COMMIT"; `
  &go get -d gotest.tools/gotestsum; `
  Set-Location "$Env:GOPATH\src\gotest.tools\gotestsum"; `
  &git checkout -q "$Env:GOTESTSUM_COMMIT"; `
  &go build --buildmode=exe; `
  Write-Host "INFO: Build done for gotestsum...";

Copy link

@vikramhh vikramhh Nov 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we are not invoking Powershell Cmdlets here but other executables, we need to explicitly check against $LASTEXITCODE. For example if you have: ARG GOTESTSUM_COMMIT=v9.3.5; the git checkout command will fail by returning the value 1. However if you depend on ErrorActionPreference value of Stop, you will fail to catch this error and the flow will continue unimpeded. You can find a detailed discussion on this at https://stackoverflow.com/questions/9948517/how-to-stop-a-powershell-script-on-the-first-error

We could definitely inline the parameters to the go and git commands but as I noted elsewhere, that is merely a matter of style/preference than anything and at this stage with this PR, I feel we should scope the changes narrowly. The same is true for inlining the function call - I would argue in favor of keeping it in a separate function because that makes the intent very clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW; looks like PowerShell 7 will support && to chain commands, which could be an option in future (although likely only for recent base images, so unlikely for RS1); see PowerShell/PowerShell#9849

Write-Host "INFO: Checkout done for gotestsum..."; `
$optsForBuild = @('"build"', '"-buildmode=exe"'); `
&go $optsForBuild; `
$savedExitCode = $LASTEXITCODE; `
} else { `
Throw '"gotestsum checkout failed..."'; `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See above comment about logs); in the failure case, we probably wouldn't have the information here what went wrong (as that info was send to the log file), making it harder to debug.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

} `
Pop-Location; `
`
if ($savedExitCode -ne 0) { `
Throw '"gotestsum build failed..."'; `
} `
Write-Host "INFO: Build done for gotestsum..."; `
} `
`
Write-Host INFO: Completed
Build-GoTestSum

# Make PowerShell the default entrypoint
ENTRYPOINT ["powershell.exe"]
Expand Down
6 changes: 4 additions & 2 deletions Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,13 +928,14 @@ pipeline {
}
post {
always {
junit testResults: 'bundles/junit-report-*.xml', allowEmptyResults: true
catchError(buildResult: 'SUCCESS', stageResult: 'FAILURE', message: 'Failed to create bundles.tar.gz') {
powershell '''
$bundleName="windowsRS1-integration"
Write-Host -ForegroundColor Green "Creating ${bundleName}-bundles.zip"

# archiveArtifacts does not support env-vars to , so save the artifacts in a fixed location
Compress-Archive -Path "${env:TEMP}/CIDUT.out", "${env:TEMP}/CIDUT.err" -CompressionLevel Optimal -DestinationPath "${bundleName}-bundles.zip"
Compress-Archive -Path "${env:TEMP}/CIDUT.out", "${env:TEMP}/CIDUT.err", "${env:TEMP}/testresults/unittests/junit-report-unit-tests.xml" -CompressionLevel Optimal -DestinationPath "${bundleName}-bundles.zip"
'''

archiveArtifacts artifacts: '*-bundles.zip', allowEmptyArchive: true
Expand Down Expand Up @@ -988,13 +989,14 @@ pipeline {
}
post {
always {
junit testResults: 'bundles/junit-report-*.xml', allowEmptyResults: true
catchError(buildResult: 'SUCCESS', stageResult: 'FAILURE', message: 'Failed to create bundles.tar.gz') {
powershell '''
$bundleName="windowsRS5-integration"
Write-Host -ForegroundColor Green "Creating ${bundleName}-bundles.zip"

# archiveArtifacts does not support env-vars to , so save the artifacts in a fixed location
Compress-Archive -Path "${env:TEMP}/CIDUT.out", "${env:TEMP}/CIDUT.err" -CompressionLevel Optimal -DestinationPath "${bundleName}-bundles.zip"
Compress-Archive -Path "${env:TEMP}/CIDUT.out", "${env:TEMP}/CIDUT.err", "${env:TEMP}/junit-report-*.xml" -CompressionLevel Optimal -DestinationPath "${bundleName}-bundles.zip"
'''

archiveArtifacts artifacts: '*-bundles.zip', allowEmptyArchive: true
Expand Down
50 changes: 46 additions & 4 deletions hack/ci/windows.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,8 @@ Try {
$errorActionPreference='Stop'
New-Item -ItemType Directory "$env:TEMP" -ErrorAction SilentlyContinue | Out-Null
New-Item -ItemType Directory "$env:TEMP\userprofile" -ErrorAction SilentlyContinue | Out-Null
New-Item -ItemType Directory "$env:TEMP\testresults" -ErrorAction SilentlyContinue | Out-Null
New-Item -ItemType Directory "$env:TEMP\testresults\unittests" -ErrorAction SilentlyContinue | Out-Null
New-Item -ItemType Directory "$env:TEMP\localappdata" -ErrorAction SilentlyContinue | Out-Null
New-Item -ItemType Directory "$env:TEMP\binary" -ErrorAction SilentlyContinue | Out-Null
New-Item -ItemType Directory "$env:TEMP\installer" -ErrorAction SilentlyContinue | Out-Null
Expand Down Expand Up @@ -525,6 +527,16 @@ Try {
if (-not($LastExitCode -eq 0)) {
Throw "ERROR: Failed to docker cp the daemon binary (dockerd.exe) to $env:TEMP\binary"
}

$GotestsumBinRelLocationInContainer="src\gotest.tools\gotestsum\gotestsum.exe"
if (-not($LastExitCode -eq 0)) {
Throw "ERROR: Failed to docker cp the gotestsum binary (gotestsum.exe) to $env:TEMP\binary"
}
docker cp "$COMMITHASH`:c`:\gopath\$GotestsumBinRelLocationInContainer" $env:TEMP\binary\
if (-not (Test-Path "$env:TEMP\binary\gotestsum.exe")) {
Throw "ERROR: gotestsum.exe not found...." `
}

$ErrorActionPreference = "Stop"

# Copy the built dockerd.exe to dockerd-$COMMITHASH.exe so that easily spotted in task manager.
Expand Down Expand Up @@ -774,11 +786,36 @@ Try {
# Run the unit tests inside a container unless SKIP_UNIT_TESTS is defined
if (($null -eq $env:LCOW_MODE) -and ($null -eq $env:LCOW_BASIC_MODE)) {
if ($null -eq $env:SKIP_UNIT_TESTS) {
$ContainerNameForUnitTests = $COMMITHASH + "_UnitTests"
Write-Host -ForegroundColor Cyan "INFO: Running unit tests at $(Get-Date)..."
$ErrorActionPreference = "SilentlyContinue"
$Duration=$(Measure-Command {docker run -e DOCKER_GITCOMMIT=$COMMITHASH$CommitUnsupported docker hack\make.ps1 -TestUnit | Out-Host })
$Duration=$(Measure-Command {docker run --name $ContainerNameForUnitTests -e DOCKER_GITCOMMIT=$COMMITHASH$CommitUnsupported docker hack\make.ps1 -TestUnit | Out-Host })
$TestRunExitCode = $LastExitCode
$ErrorActionPreference = "Stop"

# Saving for artifacts......
$unitTestsContPath="$ContainerNameForUnitTests`:c`:\gopath\src\github.com\docker\docker\bundles"
$JunitExpectedContFilePath = "$unitTestsContPath\junit-report-unit-tests.xml"
docker cp $JunitExpectedContFilePath "$TEMPORIG"
if (-not($LastExitCode -eq 0)) {
Throw "ERROR: Failed to docker cp the unit tests report ($JunitExpectedContFilePath) to $TEMPORIG"
}

if (Test-Path "$TEMPORIG\junit-report-unit-tests.xml") {
Write-Host -ForegroundColor Magenta "INFO: Unit tests results($TEMPORIG\junit-report-unit-tests.xml) exist. pwd=$pwd"
} else {
Write-Host -ForegroundColor Magenta "ERROR: Unit tests results($TEMPORIG\junit-report-unit-tests.xml) do not exist. pwd=$pwd"
}

# Saving where jenkins will take a look at.....
$bundlesDir = "bundles"
New-Item -Force -ItemType Directory $bundlesDir | Out-Null
docker cp $JunitExpectedContFilePath "$bundlesDir"
if (-not($LastExitCode -eq 0)) {
Throw "ERROR: Failed to docker cp the unit tests report ($JunitExpectedContFilePath) to $bundlesDir"
}

if (-not($TestRunExitCode -eq 0)) {
Throw "ERROR: Unit tests failed"
}
Write-Host -ForegroundColor Green "INFO: Unit tests ended at $(Get-Date). Duration`:$Duration"
Expand Down Expand Up @@ -830,7 +867,9 @@ Try {
$env:OrigDOCKER_HOST="$env:DOCKER_HOST"

#https://blogs.technet.microsoft.com/heyscriptingguy/2011/09/20/solve-problems-with-external-command-lines-in-powershell/ is useful to see tokenising
$c = "go test "
$jsonFilePath = "..\\bundles\\go-test-report-intcli-tests.json"
$xmlFilePath = "..\\bundles\\junit-report-intcli-tests.xml"
$c = "gotestsum --format=standard-quiet --jsonfile=$jsonFilePath --junitfile=$xmlFilePath -- "
$c += "`"-test.v`" "
if ($null -ne $env:INTEGRATION_TEST_NAME) { # Makes is quicker for debugging to be able to run only a subset of the integration tests
$c += "`"-test.run`" "
Expand Down Expand Up @@ -864,10 +903,12 @@ Try {
if (!($env:INTEGRATION_TESTFLAGS)) {
$env:INTEGRATION_TESTFLAGS = "-test.v"
}
Set-Location "$env:SOURCES_DRIVE`:\$env:SOURCES_SUBDIR\src\github.com\docker\docker"
$start=(Get-Date); Invoke-Expression ".\hack\make.ps1 -TestIntegration"; $Duration=New-Timespan -Start $start -End (Get-Date)
$IntTestsRunResult = $LastExitCode
$ErrorActionPreference = "Stop"
if (-not($LastExitCode -eq 0)) {
# Copy all the test results to TEMPORIG for archival
Copy-Item -Path "$env:SOURCES_DRIVE`:\$env:SOURCES_SUBDIR\src\github.com\docker\docker\bundles\junit-report*xml" -Destination $TEMPORIG
if (-not($IntTestsRunResult -eq 0)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise this was already there, just curious; is there any difference between this, and

if ($IntTestsRunResult -ne 0) {

Copy link

@vikramhh vikramhh Nov 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This piece of code is something we need to revisit immediately after these changes go through (for the issue where our Unix output does not match the actual results of test runs [passed or failed]). So if you feel strongly about using a particular pattern, please document it and we can use it for that change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not a big deal for now, but -ne 0 is easier to grasp than the "double negative" used currently.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely - will make sure to change it with that fix.

Throw "ERROR: Integration API tests failed at $(Get-Date). Duration`:$Duration"
}

Expand All @@ -877,6 +918,7 @@ Try {
Set-Location "$env:SOURCES_DRIVE`:\$env:SOURCES_SUBDIR\src\github.com\docker\docker\integration-cli"
# Explicit to not use measure-command otherwise don't get output as it goes
$start=(Get-Date); Invoke-Expression $c; $Duration=New-Timespan -Start $start -End (Get-Date)
Copy-Item -Path $xmlFilePath -Destination $TEMPORIG
}
$ErrorActionPreference = "Stop"
if (-not($LastExitCode -eq 0)) {
Expand Down
43 changes: 30 additions & 13 deletions hack/make.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ param(
$ErrorActionPreference = "Stop"
$ProgressPreference = "SilentlyContinue"
$pushed=$False # To restore the directory if we have temporarily pushed to one.
Set-Variable GOTESTSUM_LOCATION -option Constant -value "$env:GOPATH/src/gotest.tools/gotestsum"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we compile gotestsum to be put in $GOBIN (which defaults to $GOPATH\bin), we don't need this variable, and we can just call gotestsum.exe ...

If would mean we have to keep track of less things, and can simplify the scripts

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this will require a separate copy to $GOBIN and introducing or at least using a new variable $GOBIN which is currently not being used anywhere in these scripts. And it will save us from having to track things in only one place. So I am afraid it will actually end up increasing complexity rather than decreasing it.


# Utility function to get the commit ID of the repository
Function Get-GitCommit() {
Expand Down Expand Up @@ -319,38 +320,51 @@ Function Run-UnitTests() {
$pkgList = $pkgList | Select-String -NotMatch "github.com/docker/docker/man"
$pkgList = $pkgList | Select-String -NotMatch "github.com/docker/docker/integration"
$pkgList = $pkgList -replace "`r`n", " "
$goTestCommand = "go test" + $raceParm + " -cover -ldflags -w -tags """ + "autogen daemon" + """ -a """ + "-test.timeout=10m" + """ $pkgList"
$goTestCommand = "$GOTESTSUM_LOCATION\gotestsum.exe --format=standard-quiet --jsonfile=bundles\go-test-report-unit-tests.json --junitfile=bundles\junit-report-unit-tests.xml -- " + $raceParm + " -cover -ldflags -w -a """ + "-test.timeout=10m" + """ $pkgList"
Write-Host "INFO: Invoking unit tests run with $goTestCommand"
Invoke-Expression $goTestCommand
if ($LASTEXITCODE -ne 0) { Throw "Unit tests failed" }
}

# Run the integration tests
Function Run-IntegrationTests() {
$env:DOCKER_INTEGRATION_DAEMON_DEST = $root + "\bundles\tmp"
$escRoot = [Regex]::Escape($root)
$env:DOCKER_INTEGRATION_DAEMON_DEST = $bundlesDir + "\tmp"
$dirs = go list -test -f '{{- if ne .ForTest `"`" -}}{{- .Dir -}}{{- end -}}' .\integration\...
$integration_api_dirs = @()
ForEach($dir in $dirs) {
$integration_api_dirs += $dir
Write-Host "Building test suite binary $dir"
go test -c -o "$dir\test.exe" $dir
}

ForEach($dir in $integration_api_dirs) {
# Normalize directory name for using in the test results files.
$normDir = $dir.Trim()
$normDir = $normDir -replace $escRoot, ""
$normDir = $normDir -replace "\\", "-"
$normDir = $normDir -replace "\/", "-"
$normDir = $normDir -replace "\.", "-"
if ($normDir.StartsWith("-"))
{
$normDir = $normDir.TrimStart("-")
}
if ($normDir.EndsWith("-"))
{
$normDir = $normDir.TrimEnd("-")
}
$jsonFilePath = $bundlesDir + "\go-test-report-int-tests-$normDir" + ".json"
$xmlFilePath = $bundlesDir + "\junit-report-int-tests-$normDir" + ".xml"
Set-Location $dir
Write-Host "Running $($PWD.Path)"
$pinfo = New-Object System.Diagnostics.ProcessStartInfo
$pinfo.FileName = "$($PWD.Path)\test.exe"
$pinfo.FileName = "gotestsum.exe"
$pinfo.WorkingDirectory = "$($PWD.Path)"
$pinfo.RedirectStandardError = $true
$pinfo.UseShellExecute = $false
$pinfo.Arguments = $env:INTEGRATION_TESTFLAGS
$pinfo.Arguments = "--format=standard-quiet --jsonfile=$jsonFilePath --junitfile=$xmlFilePath -- $env:INTEGRATION_TESTFLAGS"
$p = New-Object System.Diagnostics.Process
$p.StartInfo = $pinfo
$p.Start() | Out-Null
$p.WaitForExit()
$err = $p.StandardError.ReadToEnd()
if (($LASTEXITCODE -ne 0) -and ($err -notlike "*warning: no tests to run*")) {
Throw "Integration tests failed: $err"
} else {
Write-Host "$err"
}
}
}
Expand All @@ -363,6 +377,11 @@ Try {
$root = $(Split-Path $MyInvocation.MyCommand.Definition -Parent | Split-Path -Parent)
Push-Location $root

# Ensure the bundles directory exists
$bundlesDir = $root + "\bundles"
Set-Variable bundlesDir -option ReadOnly
New-Item -Force $bundlesDir -ItemType Directory | Out-Null
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this use a relative path?

New-Item -Force ".\bundles" -ItemType Directory | Out-Null

Copy link

@vikramhh vikramhh Nov 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean we will need to set this in other places anyways where we cannot use a relative path[look at the function Run-IntegrationTests for example]. And it would also hide the intent that it is exactly the same directory that is being referred to in all of those places. The way it is used now, there are no dependencies that would need to be considered by someone modifying this code in the future, nor does anyone reading the code need to refer to any other part of the code to know where the logs for integration tests are being placed. So I suggest we keep it as-is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I see; it's not a "local" variable, but used globally. Wondering if we could somehow set it at the top; is there a convention to distinguish "local" from "global" variables in PowerShell? (I see other variables use an Uppercase) (or perhaps even a const?)

Copy link

@vikramhh vikramhh Nov 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are actually at the very top at this point - everything before this is functions. This variable is in global scope and one can indeed distinguish locals and globals. I chose to let it stay as a global - another alternative would have been to use it as a local and an argument to Run-IntegrationTests

Powershell does have constants but one cannot make a value constant after definition. One can however make it "Read Only" anytime - which is what I did in line 382. We could also do the same with other variables like $root which are set and then never changed.


# Handle the "-All" shortcut to turn on all things we can handle.
# Note we expressly only include the items which can run in a container - the validations tests cannot
# as they require the .git directory which is excluded from the image by .dockerignore
Expand Down Expand Up @@ -424,8 +443,6 @@ Try {

# Build the binaries
if ($Client -or $Daemon) {
# Create the bundles directory if it doesn't exist
if (-not (Test-Path ".\bundles")) { New-Item ".\bundles" -ItemType Directory | Out-Null }

# Perform the actual build
if ($Daemon) { Execute-Build "daemon" "daemon" "dockerd" }
Expand Down