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

ci: improve Go test steps #28

Merged
merged 1 commit into from
Jan 11, 2021
Merged

ci: improve Go test steps #28

merged 1 commit into from
Jan 11, 2021

Conversation

fatih
Copy link
Member

@fatih fatih commented Jan 11, 2021

The following improvements have been made:

  • Updated the Go version from 1.13 to 1.15. This is done for two reasons. First, we should always aim to use the latest version, this is a common theme in Go projects, and each version provides more performance or overall UX improvements we need. Second, starting with Go 1.14, the go command line CLI now automatically detects the vendor/ folder and uses that during go build and go test. This is needed, so the CI detects any inconsistencies during a pull request and that the source code is 100% buildable.

  • I removed the Get dependencies part because we're vendoring the source code, and go get makes a network call to fetch dependencies. We don't need to fetch the dependencies again because our "source of truth" is the vendor/ folder. This particular step also breaks our CI if we start using private dependencies (i.e., Extract psapi into github.com/planetscale/planetscale-go #27). We could configure our CI to make go get work easily, but I think we don't need it for now due to the usage of the vendor/ folder. (nit: we might need a private key in the future to verify the vendor folder + go.mod changes, but the ability to verify it is not yet implemented by the Go team cmd/go: allow verifying vendored code golang/go#27348)

  • I removed the -v flag from go test -v. The main reason is that -v outputs too much information. It's not needed. The best outcome is to output only the issues we see. The benefit is, it's easier to parse the CI output for humans, and we don't have to scroll hundreds of lines.

Before:

go test -v ./...
=== RUN   TestVerifyDevice
=== RUN   TestVerifyDevice/returns_device_verification_when_authentication_is_successful
=== RUN   TestVerifyDevice/returns_device_verification_with_check_interval_of_5_seconds_when_interval_is_0
--- PASS: TestVerifyDevice (0.00s)
    --- PASS: TestVerifyDevice/returns_device_verification_when_authentication_is_successful (0.00s)
    --- PASS: TestVerifyDevice/returns_device_verification_with_check_interval_of_5_seconds_when_interval_is_0 (0.00s)
PASS
ok      github.com/planetscale/cli/auth (cached)
?       github.com/planetscale/cli/cmd/psctl    [no test files]
?       github.com/planetscale/cli/cmdutil      [no test files]
?       github.com/planetscale/cli/config       [no test files]
?       github.com/planetscale/cli/pkg/cmd      [no test files]
?       github.com/planetscale/cli/pkg/cmd/auth [no test files]
?       github.com/planetscale/cli/pkg/cmd/database     [no test files]
=== RUN   TestDo
=== RUN   TestDo/returns_an_HTTP_response_and_no_error_for_2xx_responses
=== RUN   TestDo/returns_ErrorResponse_for_4xx_errors
=== RUN   TestDo/returns_an_HTTP_response_200_when_posting_a_request
--- PASS: TestDo (0.00s)
    --- PASS: TestDo/returns_an_HTTP_response_and_no_error_for_2xx_responses (0.00s)
    --- PASS: TestDo/returns_ErrorResponse_for_4xx_errors (0.00s)
    --- PASS: TestDo/returns_an_HTTP_response_200_when_posting_a_request (0.00s)
PASS
ok      github.com/planetscale/cli/psapi        (cached)
?       github.com/planetscale/cli/testutil     [no test files]

After:

go test ./...
ok      github.com/planetscale/cli/auth (cached)
?       github.com/planetscale/cli/cmd/psctl    [no test files]
?       github.com/planetscale/cli/cmdutil      [no test files]
?       github.com/planetscale/cli/config       [no test files]
?       github.com/planetscale/cli/pkg/cmd      [no test files]
?       github.com/planetscale/cli/pkg/cmd/auth [no test files]
?       github.com/planetscale/cli/pkg/cmd/database     [no test files]
ok      github.com/planetscale/cli/psapi        (cached)
?       github.com/planetscale/cli/testutil     [no test files]

The following improvements have been made:

* Updated the Go version from 1.13 to 1.15. This is done for two reason.
  First, we should always aim to use the latest version, this is a
  common theme in Go projects and each version provides more performance
  or overall UX improvements we need. Second, starting with Go 1.14, the
  `go` commandline CLI now automatically detects the `vendor/` folder
  and uses that during `go build` and `go test`. This is needed so the
  CI detects any inconsistencies during a pull request and that the
  source code is 100% buildable.

* I removed the `Get dependencies` part because we're vendoring the
  source code and `go get` makes a network call to fetch dependencies.
  We don't need to fetch the dependencies again, because our source of
  truth is the `vendor/` folder. Also, this particular step breaks our
  CI if we start using private dependencies (i.e:
  #27). We could configure our CI
  to easily so `go get` could work easily, but I'm not sure we need it
  at all due the usage of `vendor/`

* I removed the `-v` flag from `go test -v`. The main reason is that
  `-v` outputs to much information. It' s not needed. The best outcome
  is to output only the issues we see. The benefit is, it's easier to
  parse the CI output for humans and we don't have to scroll hundreds of
  lines.

(nit: the pictures are attached to the GitHub Pull Request, please find
the appropriate PR and check that it out there :))

Before:

```
```

After:

```
```
Copy link
Member

@iheanyi iheanyi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@fatih fatih merged commit ab05501 into main Jan 11, 2021
@fatih fatih deleted the fatih/improve-ci-go branch January 11, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants