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

Replace "go test" with "gotestsum" in Makefile #2429

Merged
merged 4 commits into from
May 22, 2023

Conversation

zalegrala
Copy link
Contributor

@zalegrala zalegrala commented May 4, 2023

What this PR does:

Here is a proposal to use gotestsum in place of go test for the purpose of improving the output of the test results. Color adds a lot of context, and the particular format chosen here is one of many available.

  • Create tools/ package and add initial go.mod
  • Imports new make targets for installing tools from sub directory
  • Include the tools target as a make dependency in several CI job tasks
  • Propose structure to support additional CI tools in the future, managed as go dependencies in tools/

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@@ -94,37 +94,37 @@ benchmark:

# Not used in CI, tests are split in pkg, tempodb, tempodb-wal and others in CI jobs
.PHONY: test-with-cover
test-with-cover: test-serverless
test-with-cover: tools test-serverless
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative to the make target dependency here, we could add make tools in the CI as a step.



# Import fragments
include build/tools.mk
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was a good time to start breaking up the make config a bit, but if we like it all in one, I can move these targets into the Makefile.

@zalegrala
Copy link
Contributor Author

The ALL_PKGS variable is empty for some reason and I've not looked why.

@zalegrala
Copy link
Contributor Author

Feel free to review a test run here, as the output will be noticeable.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

i like this. one question below. also can we add golangci-lint and jsonnetfmt to the tools list?

does gofmt come with go? or should we add that to?

tools/tools.go Show resolved Hide resolved
@zalegrala
Copy link
Contributor Author

I agree other tools can be slotted in here, perhaps in a follow up PR. I like gofumpt from https://github.com/mvdan/gofumpt but we should chat before introducing it unless we make it automatic. See my slack comment from earlier today :)


require (
github.com/psampaz/go-mod-outdated v0.9.0
gotest.tools/gotestsum v1.10.0
Copy link
Member

Choose a reason for hiding this comment

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

q: why gotest.tools, and why not github? imo, I would trust github over gotest.tools to not serve me a bad or compromised verison of gotestsum :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure, but its in their recommendation for go install, so that's what I'd put here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And indeed their module is gotest.tools also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged, since this wasn't a blocking comment, but if we want to discuss and revisit I'm open to it.

@zalegrala zalegrala merged commit 292662b into grafana:main May 22, 2023
14 checks passed
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

3 participants