-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 upgrade test against previous server version to prevent regressions #6896
Conversation
/kind misc |
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.
Quick note, slightly related to this but not entirely: the e2e-tests-upgrade.sh
runs based of latest releases (so 0.49 today for example) but not from latest LTS upgrade, which I think, would be interesting/important as well 👼🏼
@@ -0,0 +1,180 @@ | |||
//go:build e2e |
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.
As is it will be executed on all go_test_e2e
run which.. might or might not be something we want ?
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.
For now I think it might actually be nice to have it with the e2e tests. Since upgrade tests aren't run as part of CI, having this test in e2e tests would ensure breaking changes to these tests aren't checked in. It might be a better solution just to run upgrade tests in CI though.
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.
@JeromeJu can you please add more info to your commit message/PR description about the types of regressions these changes are intended to catch?
@@ -0,0 +1,180 @@ | |||
//go:build e2e |
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.
For now I think it might actually be nice to have it with the e2e tests. Since upgrade tests aren't run as part of CI, having this test in e2e tests would ensure breaking changes to these tests aren't checked in. It might be a better solution just to run upgrade tests in CI though.
384459a
to
8d32c04
Compare
test/upgrade_test.go
Outdated
} | ||
|
||
expectedTaskRun := getExpectedTaskRunUpgrade(t, taskRunName, namespace, pipelineRunName, task1Name) | ||
if d := cmp.Diff(expectedTaskRun, r, append([]cmp.Option{filterV1TaskRunSA, filterV1StepState, filterOwnerReference, filterV1TaskRunStatus, filterV1SidecarState, filterVolumeMountsName, filterTaskRunStatusProvenance}, filterV1TaskRunFields...)...); d != "" { |
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.
what does filterV1TaskRunStatus ignore? same with filterV1PipelineRunStatus. it sounds like it ignores the whole status; is it actually specific fields?
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.
These are the filters in test dir for ignoring StartTime and CompletionTime at
pipeline/test/conversion_test.go
Line 41 in c97a7e6
filterV1beta1PipelineRunStatus = cmpopts.IgnoreFields(v1beta1.PipelineRunStatusFields{}, "StartTime", "CompletionTime") |
I changed it to filterTaskRunStatusFields into this file.
This commit adds an upgrade test cases to test against previous server version by adding a light-weight test checking the existing functionalities of simple resources so as to prevent regressions introduced in the current release. It creates a pipelineRun with a basic TaskRun and a basic Pipeline with a Simple PipelineTask and examines the runs created are successful. This helps prevent the regression where the client has introduced regressions towards the last release i.e. [Regression: PipelineTaskRunSpec.Metadata incompatible with older webhooks](tektoncd#4913)
8a1d2ef
to
2c0bdd3
Compare
} | ||
|
||
expectedPipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(expectedSimplePipelineRunYaml, pipelineRunName, namespace, pipelineName, taskRunName)) | ||
if d := cmp.Diff(expectedPipelineRun, pr, append([]cmp.Option{filterV1PipelineRunStatus, filterV1PipelineRunSA, filterPipelineRunStatusFields}, filterV1PipelineRunFields...)...); d != "" { |
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.
if d := cmp.Diff(expectedPipelineRun, pr, append([]cmp.Option{filterV1PipelineRunStatus, filterV1PipelineRunSA, filterPipelineRunStatusFields}, filterV1PipelineRunFields...)...); d != "" { | |
if d := cmp.Diff(expectedPipelineRun, pr, filterV1PipelineRunStatus, filterV1PipelineRunSA, filterPipelineRunStatusFields, filterV1PipelineRunFields...); d != "" { |
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.
I am not sure if this could work according to https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff seems we need to take in a slice instead of expanding it 🤔
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.
If filterV1PipelineRunFields
is an array, then I think it can't work as suggested @lbernick (because of Go 😅 )
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick 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 |
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
Changes
This commit adds an upgrade test cases to test against previous server
version by adding a light-weight test checking the existing functionalities of
simple resources so as to prevent regressions introduced in the current
release. It creates a pipelineRun with a basic TaskRun and a basic
Pipeline with a Simple PipelineTask and examines the runs created are successful.
This helps prevent the regression where the client has introduced regressions towards the last release i.e. Regression: PipelineTaskRunSpec.Metadata incompatible with older webhooks
fixes: #5782
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes