Add Exe matcher for Windows releases. #46

interpeix · 2021-03-03T11:47:18Z

Add .Exe to extensions matcher function, to be able to download windows releases.

marcosnils · 2021-03-03T12:54:46Z

hey!, thx for the contribution. Mind adding a test for this case here?.

https://github.com/marcosnils/bin/blob/main/pkg/assets/assets_test.go#L7

Thx!

marcosnils · 2021-03-03T14:43:06Z

hey!, thx for the contribution. Mind adding a test for this case here?.

Just realized that we already had a test case for this. Is this ready to be merged @interpeix ? LMK so I can merge and release

interpeix · 2021-03-03T15:49:55Z

Sorry @marcosnils , I made a mistake and push another change related to fix the .cmd file generated by the docker provider in the same branch with this PR still opened. How do you want me to proceed? Close this one and open another two PR?
Regarding the tests, I found another problem with the runtime.GOOS value, because if i'm running on windows all linux related tests fails, so we need a way to divide the tests by the platform your machine is running on.

marcosnils · 2021-03-03T15:51:52Z

Close this one and open another two PR?

It's not really necessary, we can include it here also.

Regarding the tests, I found another problem with the runtime.GOOS value, because if i'm running on windows all linux related tests fails, so we need a way to divide the tests by the platform your machine is running on.

Hmm that makes sense.. Since I didn't have any tests when I first started the project I wasn't thinking a lot about running tests in windows. Is there a chance you can help me with that?

interpeix · 2021-03-03T15:55:41Z

I can try to find a way to test it. Maybe not in an elegant way, but i promise to do my best 😄 .

Ignore the vscode devcontainer configuration

interpeix · 2021-03-03T21:28:31Z

@marcosnils take a look at my latest changes to be able to test in both platforms.

marcosnils · 2021-03-03T21:32:12Z

pkg/assets/assets_test.go

+	//TODO: For sure there will be a better option to divide/adapt tests based on OS
+	//with this first iteration we can declare scenarios for windows and other OS
+	//without modifiying the core code of the test.
+	if runtime.GOOS == "windows" {


Just wondering if there's a reason of not using build flags and doing this instead 🤔

You're right, I have read a bit about it before propose this. I'm not yet confident using those flags. Let me dig a bit more on that idea, or if you have any recommendation feel free to give me some advice 😄

Let me dig a bit more on that idea, or if you have any recommendation feel free to give me some advice

Sure, if you check the stdlib, there are several examples on how they use build flags for tests on different platforms. i.e https://github.com/golang/go/blob/master/src/os/error_windows_test.go

Ok, IIRC we can use build tags and build flags.
With flags we can do something like this:

var windows = flag.Bool("windows", false, "only perform windows tests") func TestSanitizeName(t *testing.T) { type sample struct { in string v string out string } var cases []sample //TODO: For sure there will be a better option to divide/adapt tests based on OS //with this first iteration we can declare scenarios for windows and other OS //without modifiying the core code of the test. if *windows { cases = []sample{ {"bin_0.0.1_Windows_x86_64.exe","0.0.1","bin.exe"}, } } else {

what it's more like the previous solution using a conditional.

With tags we need to make two separate test files, for example: assets_test.go and assets_windows_test.go and put the special comment at the top of the file.
assets_test.go
//+build !windows

assets_windows_test.go
//+build windows

With tags we need to make two separate test files, for example: assets_test.go and assets_windows_test.go and put the special comment at the top of the file.
assets_test.go

This is what I originally was referring to when I mentioned build flags. This is usually the standard way of doing multi-os tests in golang generally

Lol..just realized I was actually typing "build flags" when what I actually meant is "build tags" as you're correctly pointing out. My apologies for the confusion. Build tags is definitely the way to go.

Also, I'm not sure if this should be the test that we should be actually splitting using tags. @interpeix when you were originally stating that current bin tests don't run in windows. Why is it actually failing? I don't see any windows specific code on this file that it would require having a specific build tag.

Lol..just realized I was actually typing "build flags" when what I actually meant is "build tags" as you're correctly pointing out. My apologies for the confusion. Build tags is definitely the way to go.
No worries 😄, I saw what you mean

Regarding the tests here is what is happening in my windows machine when I run it:

go test ./... ? github.com/marcosnils/bin [no test files] ? github.com/marcosnils/bin/cmd [no test files] --- FAIL: TestSanitizeName (0.00s) assets_test.go:22: Error replacing bin_amd64_linux: bin_linux does not match bin Multiple matches found, please select one: [1] bin_0.0.1_linux_x86_64 [2] bin_0.0.1_darwin_x86_64 Select an option: --- FAIL: TestFilterAssets (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal 0xc0000005 code=0x0 addr=0x18 pc=0x124cb92] goroutine 7 [running]: testing.tRunner.func1.2(0x127f840, 0x1481480) C:/Program Files/Go/src/testing/testing.go:1144 +0x345 testing.tRunner.func1(0xc000039500) C:/Program Files/Go/src/testing/testing.go:1147 +0x4b6 panic(0x127f840, 0x1481480) C:/Program Files/Go/src/runtime/panic.go:965 +0x1c7 github.com/marcosnils/bin/pkg/assets.TestFilterAssets(0xc000039500) D:/projects/bin-fork/pkg/assets/assets_test.go:70 +0x832 testing.tRunner(0xc000039500, 0x12d7270) C:/Program Files/Go/src/testing/testing.go:1194 +0xef created by testing.(*T).Run C:/Program Files/Go/src/testing/testing.go:1239 +0x2b3 FAIL github.com/marcosnils/bin/pkg/assets 0.058s ? github.com/marcosnils/bin/pkg/config [no test files] ? github.com/marcosnils/bin/pkg/options [no test files] ok github.com/marcosnils/bin/pkg/providers (cached) ? github.com/marcosnils/bin/pkg/strings [no test files] FAIL

Debugging the test I think the problem is at:

for _, c := range cases { if n := SanitizeName(c.in, c.v); n != c.out { t.Fatalf("Error replacing %s: %s does not match %s", c.in, n, c.out) } }

Inside the SanitizeName function there is a GetOS call:

func SanitizeName(name, version string) string { name = strings.ToLower(name) replacements := []string{} // TODO maybe instead of doing this put everything in a map (set) and then // generate the replacements? IDK. firstPass := true for _, osName := range config.GetOS() {

And the GetOS function:

func GetOS() []string { return []string{runtime.GOOS} }

So, the Test wants that GetOS function return "linux", but in my system that case is impossible, it always return "windows"

Maybe I made something wrong, what do you think?

I just spent a bit more time into this and we should definitely not have to use build tags to improve these tests. The tests are effectively checking the name sanitization and the filtering, so we should be able to test this independently of the OS and Arch that they're being executed on. I'll send an update today to make this code more testable by including a mock of the GetOS and GetArch resolvers 🙏

Cool!, I think that will be a better solution.

Done, check #47

This reverts commit 94cb6e6.

This reverts commit 0b639e1.

* Remove binaries by name * Reuse logic for finding binary path * make getBinPath check if arg is a path Co-authored-by: Cristian Dominguez <cristiand391@users.noreply.github.com>

…resolver

Mock resolver

interpeix · 2021-03-09T06:07:34Z

@marcosnils do you want me to change the base of the PR to feat/mock-server?

Allow to mock OS and Arch for asset filtering tests

sirlatrom · 2021-03-09T09:31:31Z

@marcosnils do you want me to change the base of the PR to feat/mock-server?

#47 is merged now so you should be able to rebase on the default branch (currently "master") to get the new mock :)

Add Exe matcher for Windows releases.

cdeb42c

fix docker sh command on Windows

4c9518b

Roberto Ferrer added 2 commits March 3, 2021 22:04

initialize sample scenarios based on OS

0b639e1

add comments to cases definition

94cb6e6

Ignore the vscode devcontainer configuration

marcosnils reviewed Mar 3, 2021

View reviewed changes

Roberto Ferrer and others added 3 commits March 5, 2021 08:48

Revert "add comments to cases definition"

96cb9a3

This reverts commit 94cb6e6.

Revert "initialize sample scenarios based on OS"

d543fff

This reverts commit 0b639e1.

Allow to mock OS and Arch for asset filtering tests

553ce8b

marcosnils mentioned this pull request Mar 5, 2021

Allow to mock OS and Arch for asset filtering tests #47

Merged

cristiand391 and others added 4 commits March 9, 2021 00:37

Remove binaries by name (marcosnils#48)

2121dde

* Remove binaries by name * Reuse logic for finding binary path * make getBinPath check if arg is a path Co-authored-by: Cristian Dominguez <cristiand391@users.noreply.github.com>

Merge remote-tracking branch 'upstream/feat/mock-resolver' into mock-…

d067356

…resolver

add resolvers and test cases

1478470

Merge pull request #1 from interpeix/mock-resolver

619550c

Mock resolver

Merge pull request marcosnils#47 from marcosnils/feat/mock-resolver

02a0be2

Allow to mock OS and Arch for asset filtering tests

Roberto Ferrer added 4 commits March 9, 2021 11:49

Add Exe matcher for Windows releases.

fd7a681

fix docker sh command on Windows

b1a559d

add resolvers and test cases

6550c9b

Merge branch 'master' of https://github.com/interpeix/bin

d050a96

interpeix requested a review from marcosnils March 10, 2021 09:06

sirlatrom added the enhancement New feature or request label Mar 12, 2021

marcosnils approved these changes Mar 15, 2021

View reviewed changes

marcosnils merged commit 27cc957 into marcosnils:master Mar 15, 2021

sirlatrom mentioned this pull request Mar 21, 2021

Bad arch #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Exe matcher for Windows releases. #46

Add Exe matcher for Windows releases. #46

interpeix commented Mar 3, 2021

marcosnils commented Mar 3, 2021

marcosnils commented Mar 3, 2021

interpeix commented Mar 3, 2021

marcosnils commented Mar 3, 2021

interpeix commented Mar 3, 2021

interpeix commented Mar 3, 2021

marcosnils Mar 3, 2021

interpeix Mar 3, 2021

marcosnils Mar 4, 2021

interpeix Mar 4, 2021

marcosnils Mar 4, 2021

marcosnils Mar 4, 2021

interpeix Mar 4, 2021

marcosnils Mar 4, 2021

interpeix Mar 5, 2021

marcosnils Mar 5, 2021

interpeix commented Mar 9, 2021

sirlatrom commented Mar 9, 2021

Add Exe matcher for Windows releases. #46

Add Exe matcher for Windows releases. #46

Conversation

interpeix commented Mar 3, 2021

marcosnils commented Mar 3, 2021

marcosnils commented Mar 3, 2021

interpeix commented Mar 3, 2021

marcosnils commented Mar 3, 2021

interpeix commented Mar 3, 2021

interpeix commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interpeix commented Mar 9, 2021

sirlatrom commented Mar 9, 2021