chore: add t.Parallel() into get_test tests

rgee0 commented 4 weeks ago

Description

There is a significant number of tool tests and each test has multiple cases. This change enables t.Parallel() in an attempt to improve the speed of execution of the tests.

Initial testing showed the elapsed time of make e2e reduced from ~12sec to ~7sec

Motivation and Context

[ ] I have raised an issue to propose this change, which has been given a label of design/approved by a maintainer (required)

How Has This Been Tested?

make e2e prior

➜  arkade git:(speedUpTests) time make e2e                                              
CGO_ENABLED=0 go test github.com/alexellis/arkade/pkg/get -cover --tags e2e -v
...
PASS
coverage: 61.5% of statements
ok      github.com/alexellis/arkade/pkg/get     12.487s coverage: 61.5% of statements
make e2e  0.91s user 1.63s system 23% cpu 12.879 total
...
PASS
coverage: 61.5% of statements
ok      github.com/alexellis/arkade/pkg/get     6.836s  coverage: 61.5% of statements
make e2e  1.18s user 1.60s system 37% cpu 7.390 total

make e2e post

➜  arkade git:(speedUpTests) time make e2e                                              
CGO_ENABLED=0 go test github.com/alexellis/arkade/pkg/get -cover --tags e2e -v
...
PASS
coverage: 61.5% of statements
ok      github.com/alexellis/arkade/pkg/get     6.836s  coverage: 61.5% of statements
make e2e  1.18s user 1.60s system 37% cpu 7.390 total

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[x] Maintenance

Documentation

[ ] I have updated the list of tools in README.md if (required) with ./arkade get --format markdown
[ ] I have updated the list of apps in README.md if (required) with ./arkade install --help

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I've read the CONTRIBUTION guide
[x] I have signed-off my commits with git commit -s

[ ] I have tested this on arm, or have added code to prevent deployment

alexellis commented 3 weeks ago

Is there a way to do this via the CLI instead, to test each of the fixtures in parallel?

alexellis commented 2 weeks ago

We took a look at this on the community call, and saw that the unit tests ran very quickly, even with -test.count set to a higher number.

make e2e will take longer due to network connections - it cannot be run in massive concurrency due to opening files, and network connections that could look like a DOS attack.

From the docs for the Go test command:

  -p n
        the number of programs, such as build commands or
        test binaries, that can be run in parallel.
        The default is the number of CPUs available.

The makefile should be updated to add i.e. -p 4 or similar to prevent the above.

Then I think it can make sense for the e2e tests to have the change, but I am not sure it's needed for the unit tests? Did you see differently?

func Test_CheckTools(t *testing.T) { already runs with a t.Parallel setting:

                t.Run("Download of "+tool.Name, func(t *testing.T) {
                        t.Parallel()
                        quiet := true

Also - we could probably refine the Makefile target for e2e to only run the fixture named Test_CheckTools

For instance, I ran this on a slow n100 machine:

time CGO_ENABLED=0 go test github.com/alexellis/arkade/pkg/get -cover --tags e2e -run "^Test_CheckTools$" -test.count=1
ok      github.com/alexellis/arkade/pkg/get 26.350s coverage: 52.1% of statements

real    0m26.651s
user    0m2.154s
sys 0m0.556s

In parallel with 32 at the same time:

time CGO_ENABLED=0 go test github.com/alexellis/arkade/pkg/get -cover --tags e2e -run "^Test_CheckTools$" -test.count=1 -p 32
ok      github.com/alexellis/arkade/pkg/get 15.121s coverage: 52.1% of statements

real    0m15.451s
user    0m2.063s
sys 0m0.588s

On my fast Ryzen machine with 16 cores 32 vCPU:

time CGO_ENABLED=0 go test github.com/alexellis/arkade/pkg/get -cover --tags e2e -run "^Test_CheckTools$" -test.count=1 -p 32
ok      github.com/alexellis/arkade/pkg/get 2.000s  coverage: 52.1% of statements

real    0m2.456s
user    0m2.509s
sys 0m0.746s

alexellis commented 2 weeks ago

Reading more, it seems like test.count - used to bust the test cache, disables parallel execution, but we have parallel execution already in place for the e2e tests.

The -parallel option could also be set, but is already set to the max number of threads/cores in a machine:

https://engineering.mercari.com/en/blog/entry/20220408-how_to_use_t_parallel/

alexellis commented 2 weeks ago

It seems like the best combination was:

time CGO_ENABLED=0 go test github.com/alexellis/arkade/pkg/get --tags e2e -run "^Test_CheckTools$" -parallel 32

alexellis / arkade