-
Notifications
You must be signed in to change notification settings - Fork 158
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
Update e2e test: timeout + ray + logging #437
Conversation
tests/e2e/controller_setup_test.go
Outdated
@@ -60,18 +60,18 @@ func NewTestContext() (*testContext, error) { | |||
// Lastly if none of them are set, it uses $HOME/.kube/config to create the client. | |||
config, err := ctrlruntime.GetConfig() | |||
if err != nil { | |||
return nil, fmt.Errorf("error creating the config object %v", err) | |||
return nil, fmt.Errorf("Error to config object %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.
@zdtsw is there a reason to upcase the starting letter in error messages? the recommended best practice is to have all error messages start with lower case.
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.
updated to lower-case
- more timeout due to new components (for in sequence run) - change validat component to run in concurrent - add Ray in the testing - logs for error message - rename functions to align names - set skip-deletion=false to test this case Signed-off-by: Wen Zhou <wenzhou@redhat.com>
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: VaishnaviHire 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 |
Description
How Has This Been Tested?
took 761s (>10m)
after change to run in parallel took 300s
Merge criteria: