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.

Sign up for GitHub

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

Jump to bottom

Add E2E Testing for Matrix #6944

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Add E2E Testing for Matrix #6944

merged 1 commit into from
Aug 1, 2023

Conversation

EmmaMunley
Copy link
Contributor

Changes

This PR adds 2 E2E Integration Tests:

  1. TestPipelineRunMatrixed is an integration test that verifies that a Matrixed PipelineRun succeeds with both matrix params and matrix include params. It also tests array indexing and whole array replacements by consuming results produced by other PipelineTasks.

  2. TestPipelineRunMatrixedFailed is an integration test with a Matrixed PipelineRun that contains 2 successful, and 1 unsucessful TaskRun. This test verifies that the 3rd task run failing will cause the PipelineRun to fail, however the first 2 TaskRuns should succeed before the last one fails.

/kind misc

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

@tekton-robot tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. release-note-none Denotes a PR that doesnt merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2023
@EmmaMunley EmmaMunley force-pushed the add-e2e-matrix-test branch 2 times, most recently from fc2cbdb to c657877 Compare July 19, 2023 16:09
@jerop
Copy link
Member

jerop commented Jul 19, 2023

Thanks for adding these tests @EmmaMunley

/test pull-tekton-pipeline-go-coverage

@EmmaMunley
Copy link
Contributor Author

/test pull-tekton-pipeline-go-coverage

jerop
jerop reviewed Jul 21, 2023
View reviewed changes
test/matrix_test.go Outdated Show resolved Hide resolved
lbernick
lbernick reviewed Jul 21, 2023
View reviewed changes
test/matrix_test.go Outdated Show resolved Hide resolved
test/matrix_test.go Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 25, 2023
@EmmaMunley EmmaMunley requested a review from jerop July 25, 2023 17:52
lbernick
lbernick reviewed Jul 25, 2023
View reviewed changes
test/matrix_test.go Outdated Show resolved Hide resolved
test/matrix_test.go Outdated Show resolved Hide resolved
test/matrix_test.go Outdated
Comment on lines 361 to 362
// 3rd task run failing will cause the PipelineRun to fail, however the first 2 TaskRuns
// should succeed before the last one fails.
Copy link
Member

Choose a reason for hiding this comment

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

What guarantees the first 2 taskruns succeed before the third fails? Does this test have the potential to be flaky if the third fails too quickly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm if I add WaitForTaskRunState(), will that ensure the TaskRuns finish in the correct order?

Copy link
Member

Choose a reason for hiding this comment

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

No, it'll just wait until the taskrun has reached the desired state, or the test times out. I don't think you can guarantee that parallel taskruns will finish in a certain order; you could add sleep statements but this can be brittle and adds latency. It's possible that the first test class you have is sufficient for e2e testing; do you think this test class covers cases that the first does not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case tests a failing TaskRun produced by a Matrix, which isn't covered in the first case. I know this is something @pritidesai wanted tested separately.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could just have the failing taskrun then, and once TEP 50 is implemented we can use that to ensure the pipeline continues even when one of the tasks fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I changed it just one taskRun checking the status that both the taskRun and PipelineRun failed.
cc: @pritidesai

Copy link
Member

Choose a reason for hiding this comment

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

If you want to continue with the original implementation, you can check the competition time of two succeeded taskRuns against the competition time of a failed task.

test/matrix_test.go Outdated Show resolved Hide resolved
test/matrix_test.go Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 26, 2023
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 31, 2023
@EmmaMunley
Add E2E Testing for Matrix
bee0412 
1) TestPipelineRunMatrixed is an integration test that verifies that a Matrixed PipelineRun succeeds with both `matrix params` and `matrix include params`. It also tests array indexing and whole array replacements by consuming results produced by other PipelineTasks.

2) TestPipelineRunMatrixedFailed is an integration test with a Matrixed PipelineRun that contains an unsuccessful TaskRun. This test verifies that the taskRun will fail, which will cause the entire PipelineRun to fail.2) TestPipelineRunMatrixedFailed is an integration test with a Matrixed PipelineRun that contains an unsuccessful TaskRun. This test verifies that the taskRun will fail, which will cause the entire PipelineRun to fail.
@EmmaMunley
Copy link
Contributor Author

/assign @pritidesai

pritidesai
pritidesai reviewed Aug 1, 2023
View reviewed changes
test/matrix_test.go
t.Fatalf("Did not get expected TaskRuns: %s", diff.PrintWantGot(d))
}

t.Logf("Successfully finished test TestPipelineRunMatrixed")
Copy link
Member

Choose a reason for hiding this comment

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

This is quite an extensive testing done, thanks a bunch for this detailed test case.

NIT: An additional or alternative way of testing this run can be done with comparing "status.childReferences[]".

pritidesai
pritidesai approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@pritidesai pritidesai left a comment

Choose a reason for hiding this comment

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

thanks @EmmaMunley 👍

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2023
@lbernick
Copy link
Member

lbernick commented Aug 1, 2023

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2023
@tekton-robot tekton-robot merged commit 94c7310 into tektoncd:main Aug 1, 2023
2 checks passed
@EmmaMunley EmmaMunley deleted the add-e2e-matrix-test branch August 1, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbernick lbernick lbernick left review comments

@pritidesai pritidesai pritidesai approved these changes

@afrittoli afrittoli Awaiting requested review from afrittoli

@jerop jerop Awaiting requested review from jerop

Assignees

@pritidesai pritidesai

@lbernick lbernick

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EmmaMunley @jerop @tekton-robot @lbernick @pritidesai