-
Notifications
You must be signed in to change notification settings - Fork 73
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 e2e tests using gRPC and REST #418
Conversation
Skipping CI for Draft Pull Request. |
/kind misc |
/test all |
/test pull-tekton-results-build-tests |
/assign @adambkaplan |
4dd599a
to
09f6bb9
Compare
if err != nil { | ||
return false, nil | ||
t.Fatalf("Error getting TaskRun: %v", err) |
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.
Shouldn't we add return false, err
here. Also line https://github.com/tektoncd/results/pull/418/files#diff-d5d12b6ea07b0a78c9ad1987e1486c565ecf16839f77a93e91b09475cf6c0e99R131 to be executed we should throw err
I think.
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 now running inside a test function that has no return. t.Fatalf
is the right thing to do.
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 the return statement of the callback function to PollImmediate
. I guess calling t.Fatal()
here would mark the test as failed before even waiting for the timeout.
if err != nil { | ||
t.Logf("Get: %v", err) | ||
return false, nil | ||
t.Fatalf("Error getting PipelineRun: %v", err) |
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.
Same add return false, err
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.
Ditto from above - we can't return any value here, and t.Fatalf
stops the test.
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 the return statement of the callback function to PollImmediate
. I guess calling t.Fatal()
here would mark the test as failed before even waiting for the timeout.
test/e2e/e2e.sh
Outdated
@@ -40,7 +40,7 @@ main() { | |||
|
|||
# Build static binaries; otherwise go test complains. | |||
export CGO_ENABLED=0 | |||
go test -v -count=1 --tags=e2e ${REPO}/test/e2e/... | |||
go test -v -count=1 --tags=e2e ${REPO}/test/e2e/e2e_test.go |
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.
Any particular reason why we should drive the test from the one file? This could lead to issues in the future if we define suites in different e2e test files.
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 have changed the behaviour to include all package in e2e
directory.
Initially this was done because the test was failing as the client
directory under e2e
doesn't have any _test
files. Now I have just excluded the client
directory from go test, while still building the files in the client
directory.
if err != nil { | ||
return false, nil | ||
t.Fatalf("Error getting TaskRun: %v", err) |
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 now running inside a test function that has no return. t.Fatalf
is the right thing to do.
if err != nil { | ||
t.Logf("Get: %v", err) | ||
return false, nil | ||
t.Fatalf("Error getting PipelineRun: %v", err) |
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.
Ditto from above - we can't return any value here, and t.Fatalf
stops the test.
/approve Marking conditional approval so other contributors can add |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan 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 |
Add e2e test for ListRecords, ListResults, GetRecord, GetResult, DeleteRecord, DeleteResult, Authorization, Authentication and Impersonation using both gRPC and REST.
@tektoncd/results-collaborators this PR is ready for an LGTM review. |
cc @tektoncd/results-maintainers |
/lgtm |
Changes
Add e2e test for ListRecords, ListResults, GetRecord, GetResult, DeleteRecord, DeleteResult, Authorization, Authentication and Impersonation using both gRPC and REST.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes