Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

bazel: native go-mockgen in Bazel #60386

Merged
merged 18 commits into from
Feb 16, 2024
Merged

bazel: native go-mockgen in Bazel #60386

merged 18 commits into from
Feb 16, 2024

Conversation

Strum355
Copy link
Contributor

@Strum355 Strum355 commented Feb 10, 2024

Adds a new:

  • gazelle generator
  • rule + rule targets + catchall target
    for generating go-mockgen mocks & testing for their being up-to-date.

Each go_mockgen macro invocation adds targets for generating mocks, copying to the source tree, as well as testing whether the current source tree mocks are up-to-date.

How to use this: bazel run //dev:go_mockgen for the catch-all, or bazel run //some/target:generate_mocks for an individual package, and bazel test //some/target:generate_mocks_tests to test for up-to-date-ness. There is no catch-all for testing

This currently uses a fork of go-mockgen, with an open PR for upstream here: derision-test/go-mockgen#50.

TODO: make go generate step local-only in sg lint

Closes https://github.com/sourcegraph/sourcegraph/issues/60099

Test plan

Extensive testing during development, including the following cases:

  • Deleting a generated file and its entry in a go_library/go_test srcs attribute list and then re-running sg bazel configure
  • Adding a non-existent output directory to mockgen.test.yaml and running the bash one-liner emitted to prepare the workspace for rerunning sg bazel configure

The existing config tests a lot of existing paths anyway (creating mocks for a 3rd party library's interface, entries for a given output file in >1 config file etc)

@Strum355 Strum355 added the bazel label Feb 10, 2024
@Strum355 Strum355 self-assigned this Feb 10, 2024
@cla-bot cla-bot bot added the cla-signed label Feb 10, 2024
@Strum355 Strum355 force-pushed the nsc/gomockgen-bazel branch from 9b43184 to b851894 Compare February 10, 2024 01:02
Copy link
Contributor

@jhchabran jhchabran left a comment

Choose a reason for hiding this comment

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

Nice! Eager to try it for myself 😊

return map[string]rule.KindInfo{
"go_mockgen": {
MatchAny: true,
// I cant tell if these work or not...
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need help for these?

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 might. I couldnt find any docs/explanation on how this is meant to work exactly, and it didnt work the way that I expected it to

@Strum355 Strum355 changed the title WIP: native go-mockgen in Bazel bazel: native go-mockgen in Bazel Feb 12, 2024
@Strum355 Strum355 marked this pull request as ready for review February 12, 2024 16:54
@Strum355 Strum355 requested a review from a team February 12, 2024 16:55
@Strum355
Copy link
Contributor Author

@jhchabran theres one other //go:generate that exists, but @burmudar agrees that its probably fine to have that one temporarily not covered by goGenerateLinter from sg lint until we move it to having a bazel target itself https://sourcegraph.com/github.com/sourcegraph/sourcegraph@nsc/gomockgen-bazel/-/blob/dev/linters/staticcheck/staticcheck.go?L1%3A36-2%3A37. Im including a commit to make goGenerateLinter onlyLocal in this PR

@Strum355 Strum355 merged commit 19d9cfc into main Feb 16, 2024
8 of 9 checks passed
@Strum355 Strum355 deleted the nsc/gomockgen-bazel branch February 16, 2024 13:26
go 1.21
go 1.21.4
Copy link
Member

@keegancsmith keegancsmith Feb 21, 2024

Choose a reason for hiding this comment

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

@Strum355 What motivated this change? I know this is valid now, but not sure why we need to be so specific on the patch version. Noticed this because the version of trivy I am using trips up on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gopls insisted on adding this and a toolchain directive immediately below that I removed, but I may have forgotten to remove this one. Feel free to remove

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate go mocks w/ Bazel
4 participants