QUA-1062: Release test reporter for windows #511

camillof · 2023-03-15T12:27:23Z

After this is merged we should be able to release a new binary for Windows.
The envy module was ~~monkey patched~~ updated in order to properly loading the GOPATH env var.

I'll update the README and public docs once we've the links for the new versions

f-moya

hey millo, we won't be able to build windows version locally right ? (if using a non-windows machine)

camillof · 2023-03-15T18:07:17Z

hey millo, we won't be able to build windows version locally right ? (if using a non-windows machine)

Yep we do! Actually, the test binary I shared on previous days was generated on my non-windows machine.

That's because we're using Golang to build them, so theoretically, you should be able to build for any architecture, from any other architecture.

dantevvp

Tested the binary on Windows 11.

format-coverage works same as linux binary
show-coverage works same as linux binary
sum-coverage works same as linux binary
upload-coverage works same as linux binary

f-moya

These changes look good to me. I have a question though, I noticed that out of all of the different formatters we have you only needed to modify the gocov formatter. Does that mean that gocov is the only formatter that needed changes to be use on a windows platform or it means that is the only formatter we will suport for windows to start ?

f-moya · 2023-03-21T19:06:06Z

.circleci/config.yml

+      - run:
+          name: Install GVM
+          command: |
+            [Net.ServicePointManager]::SecurityProtocol = "tls12"
+            Invoke-WebRequest -URI https://github.com/andrewkroh/gvm/releases/download/v0.5.0/gvm-windows-amd64.exe -Outfile gvm.exe
+      - run:
+          name: Install golang 1.15
+          command: |
+            .\gvm install 1.15
+            .\gvm --format=powershell 1.15 | Invoke-Expression
+            go version


I'm surprise we need to install go manually. Can't we use something like https://circleci.com/developer/orbs/orb/cci-orb/golang ?

Actually, the image that CircleCI provides for windows, already include go. Here I'm installing it manually because we need that specific version (1.15).

Regarding using other orbs, from what I've found, they use the same windows base image. For example the one you shared:

machine: image: 'windows-server-2019-vs2019:<< parameters.version >>' shell: << parameters.shell >> parameters: shell: default: powershell.exe -ExecutionPolicy Bypass description: Shell to use for execution command. type: string version: default: stable description: The machine image version for windows platform. type: string

They allow you to choose the image version, but none of the windows image versions include the Golang version we want (they are all way newer). And, for example, the command they include for installing other go versions, doesn't support windows (look at the source here).
In addition to that, we're using the official CircleCI orb, which I feel it may be more convenient.

f-moya · 2023-03-21T19:49:49Z

vendor/github.com/gobuffalo/envy/envy.go

+	// set the GOPATH if not present
+	if os.Getenv("GOPATH") == "" {
+		out, err := exec.Command("go", "env", "GOPATH").Output()
 		if err == nil {
-			home, err := homedir.Expand(home)
-			if err == nil {
-				os.Setenv("GOPATH", filepath.Join(home, "go"))
-			}
+			gp := strings.TrimSpace(string(out))
+			os.Setenv("GOPATH", gp)


So, you propose to; for when the GOPATH environment variable isn't set, instead of setting it to filePath.Join(homedir.Dir(), "go") use the output of go env GOPATH. However if locally I run go help env I see:

usage: go env [-json] [-u] [-w] [var ...]

Env prints Go environment information.

By default env prints information as a shell script
(on Windows, a batch file). If one or more variable
names is given as arguments, env prints the value of
each named variable on its own line.

If I'm reading that correctly it means that go env GOPATH will print the value set for the GOPATH, and yet we are trying to set something that appears to be set ? . I'm confused 😕 .

What the script is currently doing makes sense, if there is no GOPATH set already, set it to a "generally used" value; <HOMEDIR>/go.

I don't see the need for removing the pre-existent condition v >= "go1.8".

What am I missing ?

If I'm reading that correctly it means that go env GOPATH will print the value set for the GOPATH, and yet we are trying to set something that appears to be set ?

Yes and no 😄 . go env GOPATH will print the value set for the GOPATH on the GO environment, which is different from the OS environment.
So basically, if GOPATH is empty at OS environment level, we copy the value from the GO environment.

This is needed because when we ask for the GoPaths, we use the GOPATH at OS level.

What the script is currently doing makes sense, if there is no GOPATH set already, set it to a "generally used" value; /go.

Yep, but that didn't aged well, you can see the source code in the envy library, and they introduced commits like this one gobuffalo/envy@c823918. You can see that in their current release their GOPATH detection is the same code I pasted here.

I don't see the need for removing the pre-existent condition v >= "go1.8".

This condition was also removed in the original source code, and also, in our case, it will be always true because we're using go1.15

oh, that calms me a lot. I'll settle if you explain me why monkey patch envy instead of updating it ?

To be honest, probably because my subpar knowledge about golang. Thanks for asking this kind of questions!

I was able to update the whole library (as you can see on the amount of lines changed 👀 ).

@dantevvp I already tested this on windows for the dotcover formatter, do you mind repeating the tests you did? just in case, please!! 🙏

camillof · 2023-03-22T12:39:34Z

These changes look good to me. I have a question though, I noticed that out of all of the different formatters we have you only needed to modify the gocov formatter. Does that mean that gocov is the only formatter that needed changes to be use on a windows platform or it means that is the only formatter we will suport for windows to start ?

The gocov formatter was the only one that failed the tests after implementing the windows test. I think it's because it's the only one that manually handle paths, and harcoding them wasn't a good idea (/ vs \ on windows).

Also, we manually tested the test reporter with other formatters (I personally tested dot net, and the ruby one was tested by Dante and Mike), so they should also work

codeclimate

The PR diff size of 22016 lines exceeds the maximum allowed for the inline comments feature.

codeclimate · 2023-03-22T15:04:00Z

Code Climate has analyzed commit 491e210 and detected 0 issues on this pull request.

View more on Code Climate.

f-moya

Just fetched your last commit and I was able to build successfully.

Thanks for addressing the comments.

dantevvp

Built and tested the binary on Windows 11. Successfully formatted, combined and uploaded two test coverage reports to a repo.

camillof added 5 commits March 14, 2023 14:34

Update config.yml for testing on windows environment

d8cb0e2

Update tests to be compatible with windows paths formats

81c53fa

Update envy vendor package for properly load GOPATH env

ea7b5e7

Update Makefile for inlcuding Windows release

608a77f

Remove unused step on test_windows job

c9de155

camillof marked this pull request as ready for review March 15, 2023 15:35

camillof requested review from a team, larkinscott and dantevvp and removed request for a team March 15, 2023 15:36

f-moya reviewed Mar 15, 2023

View reviewed changes

dantevvp approved these changes Mar 21, 2023

View reviewed changes

f-moya reviewed Mar 21, 2023

View reviewed changes

camillof requested a review from f-moya March 22, 2023 12:34

Update envy dep instead of monkey patching it

491e210

codeclimate bot reviewed Mar 22, 2023

View reviewed changes

f-moya approved these changes Mar 22, 2023

View reviewed changes

dantevvp self-requested a review March 24, 2023 14:13

dantevvp approved these changes Mar 24, 2023

View reviewed changes

camillof merged commit d743fd1 into master Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUA-1062: Release test reporter for windows #511

QUA-1062: Release test reporter for windows #511

camillof commented Mar 15, 2023 •

edited

Loading

f-moya left a comment

camillof commented Mar 15, 2023

dantevvp left a comment

f-moya left a comment

f-moya Mar 21, 2023

camillof Mar 22, 2023

f-moya Mar 21, 2023

camillof Mar 22, 2023

f-moya Mar 22, 2023

camillof Mar 22, 2023

camillof commented Mar 22, 2023

codeclimate bot left a comment

codeclimate bot commented Mar 22, 2023

f-moya left a comment

dantevvp left a comment

QUA-1062: Release test reporter for windows #511

QUA-1062: Release test reporter for windows #511

Conversation

camillof commented Mar 15, 2023 • edited Loading

f-moya left a comment

Choose a reason for hiding this comment

camillof commented Mar 15, 2023

dantevvp left a comment

Choose a reason for hiding this comment

f-moya left a comment

Choose a reason for hiding this comment

f-moya Mar 21, 2023

Choose a reason for hiding this comment

camillof Mar 22, 2023

Choose a reason for hiding this comment

f-moya Mar 21, 2023

Choose a reason for hiding this comment

camillof Mar 22, 2023

Choose a reason for hiding this comment

f-moya Mar 22, 2023

Choose a reason for hiding this comment

camillof Mar 22, 2023

Choose a reason for hiding this comment

camillof commented Mar 22, 2023

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 22, 2023

f-moya left a comment

Choose a reason for hiding this comment

dantevvp left a comment

Choose a reason for hiding this comment

camillof commented Mar 15, 2023 •

edited

Loading