Improve taskref_test coverage with more error test cases #6813
Conversation
tekton-robot
added
kind/misc
tekton-robot
added
the
size/M
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/retest |
tekton-robot
added
the
needs-rebase
JeromeJu
force-pushed
the
refactor-taskref-test
branch
2 times, most recently
from
9b9991e
to
62bdc0e
Compare
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| t.Fatal("Expected error but found nil instead") | ||
| } | ||
| if tc.wantErr.Error() != err.Error() { | ||
| t.Fatalf("Received different error ( %#v )", err) |
There was a problem hiding this comment.
is it a typo (%#v)? We can also print the expected error to make it easier to debug.
There was a problem hiding this comment.
I think %#v is a Go-syntax representation of the value and this was previously used at
.
There was a problem hiding this comment.
nit: can we just use cmp.Diff to compare the expected error to the received error? Alternatively, can we swap back to a bool wantErr to avoid having to change the tests every time we change the error messages?
There was a problem hiding this comment.
I personally lean towards that we should expect to change the error messages in case where we have changed the logics of some errors and there could be changes in sequence of errors being emitted.
There was a problem hiding this comment.
In that case, can we use cmp.Diff to compare the expected error to the received error? See also #5882 for more context on why I suggested avoiding comparing error strings (tldr: it's brittle)
|
Thanks, lgtm! |
|
/lgtm |
This commit adds test cases with errors to improve taskref_test coverage with clusterTask test cases.
JeromeJu
force-pushed
the
refactor-taskref-test
branch
from
62bdc0e
to
3f51106
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| t.Fatal("Expected error but found nil instead") | ||
| } | ||
| if tc.wantErr.Error() != err.Error() { | ||
| t.Fatalf("Received different error ( %#v )", err) |
There was a problem hiding this comment.
In that case, can we use cmp.Diff to compare the expected error to the received error? See also #5882 for more context on why I suggested avoiding comparing error strings (tldr: it's brittle)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick, Yongxuanzhang 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 |
tekton-robot
added
the
approved
|
/lgtm |
|
/test pull-tekton-pipeline-go-coverage-df |
|
@JeromeJu: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/retest |
1 similar comment
|
/retest |
|
/test pull-tekton-pipeline-build-tests |
Changes
This commit adds test cases with errors to improve taskref_test coverage with clusterTask test cases.
/kind misc
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