-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Add integration test for release notes tool #9617
🌱 Add integration test for release notes tool #9617
Conversation
0bed7b6
to
c82d49c
Compare
c82d49c
to
5c5315b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: e0bb13ca8b31bdd9659584af2b79b8ec7979d4b0
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
Nothing major, some minor comments:
5c5315b
to
3222ba5
Compare
3222ba5
to
ce3a7f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: 49688c510b9bd92059bce7fd8a2c25977613d2a5
|
ce3a7f0
to
52da748
Compare
52da748
to
d8bef5c
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't given this a code level review - but a couple of questions.
1 - could we have this entire test in Go instead of using a shell script + the golden files + a go file?
2 -We should add a make
target and try to run it as a normal part of our test suite if possible.
I can give that a try. I can probably get rid of the bash script by doing all the setup in go running git commands. However, what's is the issue you see with golden files? We still need something to compare the output with.
The problem with running this in CI is it depends on local git (it uses worktrees to create an "replicable" environment). I'm not sure if the git tree is available when running tests, although we can probably configure it if necessary. In addition, it depends on having gh with a valid Github authentication, so it won't be trivial to automate. Given the complexity of automating this, I would prefer if we move forward with this PR and have the CI addition as in improvement for the next release cycle. At least that gives us something to verify changes against in the meantime, even if it has to be run manually. In addition, if we merge #9618, we remove the dependency on git, which should make automating this easier. |
d8bef5c
to
ad5c70c
Compare
ad5c70c
to
445370b
Compare
@@ -9,6 +9,7 @@ replace sigs.k8s.io/cluster-api/test => ../../test | |||
require ( | |||
cloud.google.com/go/storage v1.35.1 | |||
github.com/blang/semver/v4 v4.0.0 | |||
github.com/onsi/gomega v1.30.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my only real change
The other modules just come from running go mod tidy
@killianmuldoon got rid of the bash script and did everything in the go test file |
Just fyi, Killian is on PTO (IIRC for another 2-3 weeks) |
/test pull-cluster-api-test-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small nits, otherwise looks good to me.
445370b
to
f2f1dea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: f263a4d38e8e583e90e5ad39bdafdd01f218a305
|
cc @CecileRobertMichon @vincepri hey folks 👋🏼 Can you please take a look and help with reviewing/merging this one? Thank you! |
PS: I think the only reason why Furkat can't approve this one is because I added a new target to the Makefile and change the go.mod (which are both outside of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, furkatgofurov7 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
We make changes to the notes tool pretty frequently, mostly around formatting and handling edge cases. However, the lack of unit tests and the difficulty to add more (given the current structure of the code and the dependency on cli tools) makes it risky to perform these changes with speed. In order to gain confidence on the change, a lot of manual testing is required which is time consuming and prone to errors. This also makes testing for regression difficult.
This PR adds some integration tests (almost e2e): it checks out the right commit, runs the tool and compares the output with a golden file. I decided to test the code directly instead of a built cli to avoid having to build the binary every time we test and also to be able to step with a debugger easily.
This is not perfect since it depends on local git (it uses worktrees to create an "replicable" environment). In addition, it depends on having
gh
with a valid Github authentication, so it won't be trivial to automate. However, it should do the trick for now so folks can run it locally when making changes to the code. If there are changes to the expected output, these will have to be included in the PR, so we can all review them and see if the match the code changes./area release
cc @kubernetes-sigs/cluster-api-release-team