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.

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

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

Open
wants to merge 1 commit into
base: master
from

Conversation

@thaJeztah
Copy link
Member

thaJeztah commented Sep 27, 2019

carry of #39971 to make it pick-up the Jenkinsfile changes

Jenkinsfile Outdated
@@ -828,6 +828,7 @@ pipeline {
}
post {
always {
junit testResults: 'bundles/junit-report.xml', allowEmptyResults: true

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Sep 27, 2019

Author Member

I can disable the allowEmptyResults for now, so that it's clear if it failed

Perhaps we should do a dir (e.g.) too see what files are present, and if the location in which it's stored is correct

@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Sep 27, 2019

one failure, which is known to be flaky; #32219

https://ci.docker.com/public/job/moby/job/PR-39998/1/execution/node/209/log/?consoleFull

01:16:22.548      --- FAIL: TestDockerSuite/TestRestartContainerwithRestartPolicy (9.00s)
01:16:22.548          cli.go:29: assertion failed: 
01:16:22.548              Command:  d:\CI\PR-39998\1\binary\docker.exe restart 84a49937203b5fa980e4a6b69ab5af98e8d363e1862ccb9caa49da62015006a5
01:16:22.548              ExitCode: 1
01:16:22.548              Error:    exit status 1
01:16:22.548              Stdout:   
01:16:22.548              Stderr:   Error response from daemon: Cannot restart container 84a49937203b5fa980e4a6b69ab5af98e8d363e1862ccb9caa49da62015006a5: Container 84a49937203b5fa980e4a6b69ab5af98e8d363e1862ccb9caa49da62015006a5 is not running
01:16:22.548              
01:16:22.548              
01:16:22.548              Failures:
01:16:22.548              ExitCode was 1 expected 0
01:16:22.548              Expected no error

It did run the archive junit results step; https://ci.docker.com/public/job/moby/job/PR-39998/1/execution/node/355/log/

But I don't see the test-results for windows;
https://ci.docker.com/public/job/moby/job/PR-39998/1/testReport/

@andrewhsu

This comment has been minimized.

Copy link
Contributor

andrewhsu commented Sep 29, 2019

To help with development iteration, you could throw in a temporary git commit to disable tests except for windowsRS5.

moby/Jenkinsfile

Lines 11 to 17 in 5b57f41

booleanParam(name: 'unit_validate', defaultValue: true, description: 'amd64 (x86_64) unit tests and vendor check')
booleanParam(name: 'amd64', defaultValue: true, description: 'amd64 (x86_64) Build/Test')
booleanParam(name: 's390x', defaultValue: true, description: 'IBM Z (s390x) Build/Test')
booleanParam(name: 'ppc64le', defaultValue: true, description: 'PowerPC (ppc64le) Build/Test')
booleanParam(name: 'windowsRS1', defaultValue: false, description: 'Windows 2016 (RS1) Build/Test')
booleanParam(name: 'windowsRS5', defaultValue: true, description: 'Windows 2019 (RS5) Build/Test')
booleanParam(name: 'skip_dco', defaultValue: false, description: 'Skip the DCO check')

@thaJeztah thaJeztah force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from 3ebfb8f to af4e179 Oct 8, 2019
@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Oct 9, 2019

Looks like it still doesn't write results in the right location;

https://ci.docker.com/public/job/moby/job/PR-39998/5/execution/node/356/log/

01:01:32.396  Recording test results
01:01:34.755  None of the test reports contained any result
[2019-10-08T17:19:36.991Z] INFO: Invoking unit tests run with C:\gopath/src/gotest.tools/gotestsum\gotestsum.exe --format=standard-quiet --jsonfile=bundles\go-test-report.json --junitfile=bundles\junit-report.xml -- -cover -ldflags -w -tags "autogen daemon" -a "-test.timeout=10m"  .........
...

[2019-10-08T17:23:07.620Z] INFO: make.ps1 ended at 10/08/2019 17:23:07
[2019-10-08T17:23:09.158Z] WARN: junit (Unit test) results(f4eccb3b36:c:\gopath\src\github.com\docker\docker\bundles\junit-report.xml) not found
[2019-10-08T17:23:09.158Z] INFO: Unit tests ended at 10/08/2019 17:23:09. Duration:00:04:10.1162257
@thaJeztah thaJeztah force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from 0595e1d to 1818270 Oct 10, 2019
@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from 1818270 to 3cb4def Oct 11, 2019
@thaJeztah thaJeztah force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from 3cb4def to 8fab97f Oct 11, 2019
@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Oct 11, 2019

@vikramhh I rebased your branch again; I think your local git was not up-to-date with the upstreams.

@thaJeztah thaJeztah added this to In progress in Improving CI via automation Oct 11, 2019
@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch 2 times, most recently from 3b8b468 to 413a18e Oct 11, 2019
@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch 5 times, most recently from 59c3482 to 7408acd Oct 19, 2019
@thaJeztah thaJeztah changed the title Build and use gotestsum for running unit tests Windows: Build and use gotestsum for running all tests Oct 20, 2019
@thaJeztah thaJeztah marked this pull request as ready for review Oct 20, 2019
@thaJeztah thaJeztah requested a review from tianon as a code owner Oct 20, 2019
Dockerfile.windows Outdated Show resolved Hide resolved
Dockerfile.windows Outdated Show resolved Hide resolved
`
Write-Host INFO: Completed
Write-Host INFO: Building gotestsum...; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 20, 2019

Author Member

Should we move this inside the Build-GoTestSum function?

This comment has been minimized.

Copy link
@vikramhh

vikramhh Oct 20, 2019

I do not know a compelling argument either way. Why would you choose one over the other?

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

All other messages (including "Build done ..") are printed as part of the script, so it would make sense to include this as well. If it's move to a script, we could just call Build-GoTestSum and be done

This comment has been minimized.

Copy link
@vikramhh
} `
} `
} `
Function Build-GoTestSum() { `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 20, 2019

Author Member

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)

This comment has been minimized.

Copy link
@vikramhh

vikramhh Oct 20, 2019

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

Jenkinsfile Outdated Show resolved Hide resolved
hack/make.ps1 Outdated Show resolved Hide resolved
ForEach($dir in $integration_api_dirs) {
#$integration_api_dirs += $dir
#Write-Host "Building test suite binary $dir"
#go test -c -o "$dir\test.exe" $dir

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 20, 2019

Author Member

Same here.

Wondering though why we're no longer building the binaries here? (perhaps you can explain why this changed)

This comment has been minimized.

Copy link
@vikramhh

vikramhh Oct 20, 2019

Instead of building the test binary and then running it, we are just running the test. Why we need to do it in two different stages?

hack/make.ps1 Outdated Show resolved Hide resolved
hack/make.ps1 Outdated Show resolved Hide resolved
$p = New-Object System.Diagnostics.Process
$p.StartInfo = $pinfo
$p.Start() | Out-Null
$p.WaitForExit()
$err = $p.StandardError.ReadToEnd()
Write-Host "$err"

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 20, 2019

Author Member

I think this is to print stderr output if there's no error that happened, correct?

In case of an error ($LASTEXITCODE -ne 0), this will now be printed twice. Think it should be changed to either;

if (($LASTEXITCODE -ne 0) -and ($err -notlike "*warning: no tests to run*")) {
    Throw "Integration tests failed: $err"
} else {
    Write-Host "$err"
}

Or (if Throw exits the script (not sure if if does so?)); perhaps add a comment as well to explain why we're printing $err

if (($LASTEXITCODE -ne 0) -and ($err -notlike "*warning: no tests to run*")) {
    Throw "Integration tests failed: $err"
}

# Write stderr output
Write-Host "$err"
@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from 7408acd to eb5e871 Oct 20, 2019
@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch 2 times, most recently from 9c500f5 to 79ebfeb Oct 28, 2019
@andrewhsu

This comment has been minimized.

Copy link
Contributor

andrewhsu commented Oct 30, 2019

I'm starting to see the junit reports of the test results in jenkins UI now, which is a great step: https://ci.docker.com/public/blue/organizations/jenkins/moby/detail/PR-39998/38/tests

@andrewhsu

This comment has been minimized.

Copy link
Contributor

andrewhsu commented Oct 30, 2019

Looks like the failures listed in the junit report are also seen in other PRs to moby: #40154 (comment)

  • TestDebugInfo
  • TestInfoInsecureRegistries
  • TestInfoRegistryMirrors
@andrewhsu

This comment has been minimized.

Copy link
Contributor

andrewhsu commented Oct 30, 2019

Workaround for now: #40155

Copy link
Member Author

thaJeztah left a comment

Thanks! Left my comments inline

FROM_DOCKERFILE=1 `
GOTESTSUM_COMMIT=${GOTESTSUM_COMMIT} `
GOTESTSUM_LOCATION="src\gotest.tools\gotestsum" `
DOCKER_SRC_REL_LOCATION="src\github.com\docker\docker" `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

nit (we can address in the follow-up), but both GOTESTSUM_LOCATION and DOCKER_SRC_REL_LOCATION are always used as ${GOPATH}\${GOTESTSUM_LOCATION}. Given that there's no need to customise these, I think I'd prefer just inlining those where they're used, so

(fake code)

<something> $Env:GOPATH\src\gotest.tools\gotestsum

Abstracting these away into variables makes it more difficult to read the code, and the locations where they're used is limited, so there's not a lot of duplication.

If we do need them in more places, we should work with relative paths, e.g.

cd $Env:GOPATH\src\docker\docker
dosomething .

This comment has been minimized.

Copy link
@vikramhh
$GotestsumPath=Join-Path -Path $Env:GOPATH -ChildPath $Env:GOTESTSUM_LOCATION; `
$GotestsumBinPath=Join-Path -Path $GotestsumPath -ChildPath "gotestsum.exe"; `
if (Test-Path $GotestsumBinPath) { `
Write-Host "INFO: gotestsum already available in $cwGotestsumBinPath - skipping building it...." `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

This is done inside the Dockerfile, so there should never be a gotestsum.exe in there. Even if there would be, we must be sure that it's the right version, so we should just build it anyway.

I suggest removing this check

This comment has been minimized.

Copy link
@vikramhh
} `
$optsForGet = @('"get"', '"-d"', '"gotest.tools/gotestsum"'); `
$logFileName = 'gotestsum.get.out'; `
&go $optsForGet > $logFileName 2>&1; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

Why the intermediate $optsForGet variable? It's only used in a single location, and makes it harder to see what we're running here. Instead, I would inline it here;

&go get -d "gotest.tools/gotestsum" > $logFileName 2>&1; `

This comment has been minimized.

Copy link
@vikramhh
$savedExitCode = $LASTEXITCODE; `
Save-FileIfNonZero -LogName $logFileName; `
} else { `
Throw '"gotestsum checkout failed..."'; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

(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.

This comment has been minimized.

Copy link
@vikramhh
Write-Host "INFO: Sources obtained for gotestsum..."; `
Push-Location $GotestsumPath; `
$optsForCheckout = @('"checkout"', '"-q"', """$Env:GOTESTSUM_COMMIT"""); `
$logFileName = 'gotestsum.git.out'; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

Are we using these logfiles anywhere? We could consider to just have it print the output so that it's captured in the Jenkins log.

These would only be printed if docker doesn't have a build-cache for this step, but makes it easier to follow what was done (albeit with some more noise). Currently the only output is;

[2019-10-28T22:36:49.879Z] INFO: Building gotestsum...
[2019-10-28T22:36:49.879Z] INFO: No existing binary of gotestsum found. Building gotestsum version v0.3.5 in C:\gopath
[2019-10-28T22:37:10.960Z] INFO: Sources obtained for gotestsum...
[2019-10-28T22:37:10.960Z] INFO: Checkout done for gotestsum...
[2019-10-28T22:37:11.417Z] INFO: Build done for gotestsum...

Which (obviously) would be more noisy if we didn't hide the logs, however, in the cached situation, those would not be shown, so that's not a problem;

[2019-10-28T23:59:15.923Z]  ---> Using cache
[2019-10-28T23:59:15.923Z]  ---> fa367ecb71ee

If we do have strong reasons to hide the output and send it to a log file;

  • change .out to .log
  • use a single file for both this checkout and build, so that there's no need to lookup two separate files and joining them together

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

I made the change so that we are not hiding the output

`
Write-Host INFO: Completed
Write-Host INFO: Building gotestsum...; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

All other messages (including "Build done ..") are printed as part of the script, so it would make sense to include this as well. If it's move to a script, we could just call Build-GoTestSum and be done

C:\git\cmd\git config --global core.autocrlf true; `
C:\git\cmd\git config --global core.autocrlf true;

WORKDIR ${GOPATH}

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

Was there a reason to change to this location here? Looks like the script takes care to going to the correct directory, so changing the WORKDIR here, only to change it back to $GOPATH\src\github.com\docker\docker after, may not be needed.

Effectively, now we do;

WORKDIR ${GOPATH}
RUN Build-GoTestSum
WORKDIR ${GOPATH}\src\docker\docker

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

Moved the change of directory to Env:GOPATH explicitly in the script and removed the first WORKDIR above.

# Copy the sources into the container
COPY . .

RUN `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

This looks redundant; docker build will already show that the build completed

This comment has been minimized.

Copy link
@vikramhh
@@ -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"

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

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

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

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.

@@ -363,6 +379,10 @@ Try {
$root = $(Split-Path $MyInvocation.MyCommand.Definition -Parent | Split-Path -Parent)
Push-Location $root

# Create the bundles directory if it doesn't exist
$bundlesDir = $root + "\bundles"
if (-not (Test-Path $bundlesDir)) { New-Item $bundlesDir -ItemType Directory | Out-Null }

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 31, 2019

Author Member

Can we change to the equivalent of mkdir -p here? Then we can also get rid of the intermediate variable

I think that's something like;

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

Actually, I see the Push-Location above, so we could probably even use a relative path (I see that was was used previously)

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

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

I did make the change to the latter by making it equivalent to "mkdir -p". However the intermediate variable is then used at other places as well. Making it an explicit value instead of the variable will require us to still set this variable in other places for those usages [or convert those in multiple places to use explicit paths]. But more important, I am afraid it will mask the intent [that it is the same directory which is being referred to in all those disparate locations] and make the code hard to maintain. So I would argue for leaving this as-is.

@andrewhsu

This comment has been minimized.

Copy link
Contributor

andrewhsu commented Oct 31, 2019

Talked with vikram about this PR today. Looks like the junit report of failures is enough to make sure the github status of the PR checks is red even though the unix return code of that step is 0. This seems acceptable to get this PR in but needs work later to get the unix return code of the step to accurately reflect test failures.

@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from 79ebfeb to ec54590 Oct 31, 2019
Copy link
Member Author

thaJeztah left a comment

I see you updated some parts, thanks! (left some comments inline); looks like it's currently failing (wondering if we need to add something to $PATH?)


[2019-10-31T23:53:17.936Z] INFO: Ensuring existence of directory C:\gopath\src\github.com\docker\docker\bundles ...
[2019-10-31T23:53:17.936Z] INFO: Configuring git core.autocrlf...
[2019-10-31T23:53:17.936Z] INFO: Building gotestsum version v0.3.5 in C:\gopath  = get -d gotest.tools/gotestsum
[2019-10-31T23:53:20.103Z] & : The term 'go' is not recognized as the name of a cmdlet, function, script 
[2019-10-31T23:53:20.103Z] file, or operable program. Check the spelling of the name, or if a path was 
[2019-10-31T23:53:20.103Z] included, verify that the path is correct and try again.
[2019-10-31T23:53:20.103Z] At line:1 char:1711
[2019-10-31T23:53:20.103Z] + ... st.tools/gotestsum'); $logFileName = 'gotestsum.get.out'; &go $optsFo ...
# 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...; `
New-Item -ItemType Directory -Path $srcDir | Out-Null; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

We should probably add -Force here (to get the equivalent of mkdir -p); we don't want this to fail if the bundles directory exists.

This comment has been minimized.

Copy link
@vikramhh
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...; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

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

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

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.

$GotestsumBinPath=Join-Path -Path $GotestsumPath -ChildPath "gotestsum.exe"; `
Write-Host "INFO: Building gotestsum version $Env:GOTESTSUM_COMMIT in $Env:GOPATH" `
$optsForGet = @('"get"', '"-d"', '"gotest.tools/gotestsum"'); `
$logFileName = 'gotestsum.get.out'; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

This variable is no longer used so can be removed

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

Done - removed all references to log file names.

Write-Host "INFO: Sources obtained for gotestsum..."; `
Push-Location $GotestsumPath; `
$optsForCheckout = @('"checkout"', '"-q"', """$Env:GOTESTSUM_COMMIT"""); `
$logFileName = 'gotestsum.git.out'; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

This variable is no longer used so can be removed

This comment has been minimized.

Copy link
@vikramhh
$savedExitCode = $LASTEXITCODE; `
if ($savedExitCode -eq 0) { `
Write-Host "INFO: Checkout done for gotestsum..."; `
$logFileName = 'gotestsum.build.out'; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

This variable is no longer used so can be removed

This comment has been minimized.

Copy link
@vikramhh
Push-Location $GotestsumPath; `
$optsForCheckout = @('"checkout"', '"-q"', """$Env:GOTESTSUM_COMMIT"""); `
$logFileName = 'gotestsum.git.out'; `
&git $optsForCheckout; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

See my previous review; could those options be moved inline here?

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

I feel that the above is much cleaner and easier to read. I am open to changing it if there is a reason why we should inline the arguments here.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

Perhaps personally, but in order to understand what this line does, I have look for where $optsForCheckout is declared; so read up two lines higher, mentally "remove" the additional quoting that's needed there, and "reconstruct" that this says;

git checkout -q "$GOTESTSUM_COMMIT"
Write-Host "INFO: Checkout done for gotestsum..."; `
$logFileName = 'gotestsum.build.out'; `
$optsForBuild = @('"build"', '"-buildmode=exe"'); `
&go $optsForBuild; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

Same here

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

Same as above - we can change it if you feel strongly for some reason that arguments must be inline only.

@@ -208,6 +211,40 @@ RUN `
} `
} `
`
Function Build-GoTestSum() { `
$GotestsumPath=Join-Path -Path $Env:GOPATH -ChildPath "src\gotest.tools\gotestsum"; `
$GotestsumBinPath=Join-Path -Path $GotestsumPath -ChildPath "gotestsum.exe"; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

Looks like this one isn't used anymore as well

This comment has been minimized.

Copy link
@vikramhh

# Saving where jenkins will take a look at.....
$bundlesDir = "bundles"
if (-not (Test-Path $bundlesDir)) { New-Item $bundlesDir -ItemType Directory | Out-Null }

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

oh; this one could also use -Force (equivalent of mkdir -p);

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

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

Done - however instead of explicit use of the value of the directory name, I am choosing to keep the variable because it is used elsewhere too.

@@ -363,6 +379,10 @@ Try {
$root = $(Split-Path $MyInvocation.MyCommand.Definition -Parent | Split-Path -Parent)
Push-Location $root

# Ensure the bundles directory exists
$bundlesDir = $root + "\bundles"
New-Item -Force $bundlesDir -ItemType Directory | Out-Null

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

Could this use a relative path?

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

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

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.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

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?)

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

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.

if ($savedExitCode -eq 0) { `
Write-Host "INFO: Checkout done for gotestsum..."; `
$logFileName = 'gotestsum.build.out'; `
$optsForBuild = @('"build"', '"-buildmode=exe"'); `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

note: with go build -o <some path> you can specify where the binary should be stored, which could make things easier to handle (you can collect the binaries in a standard directory)

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)) {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

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

if ($IntTestsRunResult -ne 0) {

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

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.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

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

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 1, 2019

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

if ($normDir.StartsWith("-"))
{
$normDir = $normDir.TrimStart("-")
Write-Host $normDir

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

Think this was for debugging

This comment has been minimized.

Copy link
@vikramhh
if ($normDir.EndsWith("-"))
{
$normDir = $normDir.TrimEnd("-")
Write-Host $normDir

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 1, 2019

Author Member

Same

This comment has been minimized.

Copy link
@vikramhh
@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from e8a561d to a3d9ba5 Nov 1, 2019

# Need a different layer because the current layer will not
# have go in its path. We could always figure that out either
# by observing the current behavior of go-s installer (and

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 2, 2019

Author Member

I'm a bit confused by this, because Go doesn't have an installer; we're downloading a zip and extract it to C:\go. We update the path ourselves at this line;

setx /M PATH $('C:\git\cmd;C:\git\usr\bin;'+$Env:PATH+';C:\gcc\bin;C:\go\bin'); `

Does Windows only evaluate PATH when the shell is started, and not when updated in the same shell?

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 2, 2019

That is correct[confirmed the behavior from a Powershell command prompt]. The below works - or just doing it in another layer, which is what I chose to do.

$env:Path += """;c:\go\bin""";
$env:Path += """;c:\git\cmd""";

My A/B testing did not produce a clear "winner" so it looks like we are good either way.

Throw '"Failed getting gotestsum sources..."' `
}; `
Write-Host "INFO: Sources obtained for gotestsum..."; `
Push-Location $GotestsumPath; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 2, 2019

Author Member

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

This comment has been minimized.

Copy link
@vikramhh
Write-Host "INFO: Sources obtained for gotestsum..."; `
Push-Location $GotestsumPath; `
$optsForCheckout = @('"checkout"', '"-q"', """$Env:GOTESTSUM_COMMIT"""); `
&git $optsForCheckout; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 2, 2019

Author Member

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

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 2, 2019

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

`
Write-Host INFO: Completed
Set-Location -Path $Env:GOPATH; `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 2, 2019

Author Member

The function immediately changes to $GotestsumPath, so likely this isn't needed.

This comment has been minimized.

Copy link
@vikramhh
$optsForCheckout = @('"checkout"', '"-q"', """$Env:GOTESTSUM_COMMIT"""); `
&git $optsForCheckout; `
$savedExitCode = $LASTEXITCODE; `
if ($savedExitCode -eq 0) { `

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Nov 2, 2019

Author Member

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:

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...";

This comment has been minimized.

Copy link
@vikramhh

vikramhh Nov 2, 2019

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.

@vikramhh vikramhh force-pushed the thaJeztah:carry_39971_unittests_junit_1 branch from a3d9ba5 to 5b8d715 Nov 2, 2019
@vikramhh

This comment has been minimized.

Copy link

vikramhh commented Nov 4, 2019

@thaJeztah - the failures on RS5 are known issues. If you are good with the changes, could we get RS1 checks as well.

@andrewhsu

This comment has been minimized.

Copy link
Contributor

andrewhsu commented Nov 7, 2019

This PR is valuable as is with RS5 junit test results in jenkins. It is important to expose the failures since the return code is always success.

I'd recommend getting this PR in as soon as possible and having a separate PR for RS1.

@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Nov 7, 2019

The issue we noticed when discussing, is that the naming of the tests for Windows is different than the ones for Linux, therefore the tests show in a different place; for reference, this is how the naming is set for the Linux ones for the junit.xml;

cd "$dir"
# Create a useful package name based on the tests's $dir. We need to take
# into account that "$dir" can be either an absolute (/go/src/github.com/docker/docker/integration/foo)
# or relative (./integration/foo) path. To account for both, first we strip
# the absolute path, then remove any leading periods and slashes.
pkgname="${dir}"
pkgname="${pkgname#*${GOPATH}/src/${DOCKER_PKG}}"
pkgname="${pkgname#*.}"
pkgname="${pkgname#*\/}"
# Finally, we use periods as separator (instead of slashes) to be more
# in line with Java package names (which is what junit.xml was designed for)
pkgname="$(go env GOARCH).${pkgname//\//.}"
echo "Running $PWD (${pkgname}) flags=${flags}"
[ -n "$TESTDEBUG" ] && set -x
# shellcheck disable=SC2086
test_env gotestsum \
--format=standard-verbose \
--jsonfile="${ABS_DEST}/${pkgname//./-}-go-test-report.json" \
--junitfile="${ABS_DEST}/${pkgname//./-}-junit-report.xml" \
--raw-command \
-- go tool test2json -p "${pkgname}" -t ./test.main ${flags}

@vikramhh

This comment has been minimized.

Copy link

vikramhh commented Nov 8, 2019

The above is true only for integration tests and could be handled by transforming the output of all tests [i.e. the xml files] by applying the same rules to each test phase [unit/integration and integration-cli] once we figure out where each result type should show up. This is not a blocker.

Copy link
Member Author

thaJeztah left a comment

LGTM

@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Nov 9, 2019

@vikramhh could you rebase this PR, because I think the failing test is fixed / skipped on master, so after rebasing, it should be green

1. Dockerfile.Windows modified to build gotestsum.exe

2. Use gotestsum.exe in invoking the execution of:

   (a) Unit tests (run in containers),
   (b) Integration tests (run outside containers)
   (c) Integration-cli (run outside containers)

No changes made to other categories of tests (e.g.
LCOW).

3. Copy .xml files produced by gotestsum in
   appropriate paths where Jenkins can ingest them

4. Modify Jenkinsfile to mark results output as
   being jUnit "type" as well as to archive the
   .xml test result files as artifacts.

Signed-off-by: Vikram bir Singh <vikrambir.singh@docker.com>
@vikramhh

This comment has been minimized.

Copy link

vikramhh commented Nov 9, 2019

Rebased - the two failures are expected because only 1 of the 3 failing tests in #40155 has been disabled. The other two still fail [even though Jenkins does not always mark the checks as failed, something that should be partially fixed by this PR]. The third failure will be fixed by #40193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Improving CI
  
In progress
3 participants
You can’t perform that action at this time.