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

Added test to check forgotten codegen updates. #427

Merged
merged 2 commits into from
Jun 28, 2023
Merged

Conversation

mwhittaker
Copy link
Member

In PR #388, we changed weaver generate to embed listener information in compiled Service Weaver binaries. This change landed in v0.15.0. However, we forgot to update the codegen version. This led to the following bug, reported by @renanbastos93 in the Discord:

  • Run weaver generate at version v0.14.0.
  • Run go run . at version v0.16.1.
  • The app crashes looking for listener information in the binary, but the information is not there because of the stale weaver generate.

If we had updated the codegen version, then the code would not have compiled, and the user would have received an error message explaining that they needed to update weaver generate.

It's really easy to forget to update the codegen version, so I added a unit test that checks for changes to how weaver_gen.go files are generated. If there is any change, the test fails and you're prompted to update the codegen version.

In PR #388, we changed `weaver generate` to embed listener information
in compiled Service Weaver binaries. This change landed in v0.15.0.
However, we forgot to update the codegen version. This led to the
following bug, reported by @renanbastos93 in the Discord:

- Run `weaver generate` at version v0.14.0.
- Run `go run .` at version v0.16.1.
- The app crashes looking for listener information in the binary, but
  the information is not there because of the stale `weaver generate`.

If we had updated the codegen version, then the code would not have
compiled, and the user would have received an error message explaining
that they needed to update `weaver generate`.

It's really easy to forget to update the codegen version, so I added a
unit test that checks for changes to how weaver_gen.go files are
generated. If there is any change, the test fails and you're prompted to
update the codegen version.
@mwhittaker mwhittaker self-assigned this Jun 28, 2023
mwhittaker added a commit that referenced this pull request Jun 28, 2023
This PR adds a test that fails when you forget to update the deployer
API version. See PR #427 for more context.
Copy link

@renanbastos93 renanbastos93 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@spetrovic77 spetrovic77 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing!

@mwhittaker mwhittaker merged commit bd33647 into main Jun 28, 2023
7 checks passed
@mwhittaker mwhittaker deleted the codegen_checks branch June 28, 2023 18:13
mwhittaker added a commit that referenced this pull request Jun 28, 2023
This PR adds a test that fails when you forget to update the deployer
API version. See PR #427 for more context.
mwhittaker added a commit that referenced this pull request Jun 28, 2023
This PR adds a test that fails when you forget to update the deployer
API version. See PR #427 for more context.
@mwhittaker
Copy link
Member Author

mwhittaker commented Jun 28, 2023

I chatted with Sanjay about this test a bit. An alternative is to have a separate script that compares the value of the weaver_gen.go files on HEAD with the ones at the latest tag (which can be fetched with go mod download -json github.com/ServiceWeaver/weaver@latest). If the weaver_gen.go files differ, but the codegen API version does not differ, then the script fails. We can run the script as part of build_and_test. We can also run apidiff to check for changes to the codegen API itself. We can do something similar for the deployer API by checking for changes in runtime.proto and the envelope API itself.

The tricky thing is that some changes to weaver_gen.go files might be inconsequential (e.g., changing variable names or adding comments), and the script would keep flagging these unimportant changes. I'll think about this some more. If anyone has ideas, let me know :)

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