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

github: Run go test in CI #201

Merged
merged 1 commit into from
Sep 7, 2022
Merged

github: Run go test in CI #201

merged 1 commit into from
Sep 7, 2022

Conversation

cole-miller
Copy link
Contributor

We already run go test on Launchpad, but not for pull requests. Not sure whether there's a reason for that, so I'm opening this PR to facilitate discussion.

Signed-off-by: Cole Miller cole.miller@canonical.com

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Is there a reason you dont use go test go-dqlite/...?

@tomponline
Copy link
Member

@markylaing I would appreciate your thoughts on this one given youve done a lot of our github ci stuff lately. Thanks

@cole-miller
Copy link
Contributor Author

cole-miller commented Sep 7, 2022

Is there a reason you dont use go test go-dqlite/...?

I'm not super familiar with the Go command-line syntax, so I just copied what Launchpad was doing. I'll simplify it, one sec...

@tomponline
Copy link
Member

Is there a reason you dont use go test go-dqlite/...?

I'm not super familiar with the Go command-line syntax, so I just copied what Launchpad was doing. I'll simplify it, one sec...

Thanks you could take a look at what we do in the lxd github repo, as that way we won't need to remember to add each sub package to the list. Probably worth updating launchpad too otherwise things may be missed there also. Thanks

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Why vet off btw?

@cole-miller
Copy link
Contributor Author

cole-miller commented Sep 7, 2022 via email

@stgraber
Copy link
Contributor

stgraber commented Sep 7, 2022

https://github.com/canonical/dqlite-ppa

@tomponline
Copy link
Member

Does go test pass with vet off removed? If so I see no reason to leave it disabled. As for launchpad I'm not sure but agree they should be in sync. @stgraber can you point us to where the launchpad build scripts are? Thanks

@tomponline
Copy link
Member

Signed-off-by: Cole Miller <cole.miller@canonical.com>
@cole-miller
Copy link
Contributor Author

Does go test pass with vet off removed?

Yes! Updated this PR. As for Launchpad, as far as I can tell, the arguments passed to go test aren't defined by us at all, they're managed by debian's packaging tools, which have their own idea of how to build and test Go projects.

@stgraber stgraber merged commit bca58cd into canonical:master Sep 7, 2022
@MathieuBordere
Copy link
Contributor

MathieuBordere commented Sep 8, 2022

overalls runs the tests, see here.

The following line in the .github actions file takes care of it.
overalls -project ${{ github.workspace }} -covermode=count -- -tags libsqlite3 -timeout 240s

$ overalls -help

usage: overalls -project=[path] -covermode[mode] OPTIONS -- TESTOPTIONS

overalls recursively traverses your projects directory structure
running 'go test -covermode=count -coverprofile=profile.coverprofile'
in each directory with go test files, concatenates them into one
coverprofile in your root directory named 'overalls.coverprofile'

@markylaing
Copy link

I had never heard of overalls before! Overall the test/lint actions in this repo look fine. We could use golangci-lint to add a few more linters like in LXD (see the static analysis target in the Makefile and the golangci-lint configuration file).

@cole-miller
Copy link
Contributor Author

Thanks @MathieuBordere, looks like we do get test failure reports from overalls, so I'll revert this.

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.

None yet

5 participants