-
Notifications
You must be signed in to change notification settings - Fork 72
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
New structural changes to test repo #531
Conversation
Hi @nitishSr. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@nitishSr I like the structural changes, LGTM. Let me know when this is ready for review |
Codecov Report
@@ Coverage Diff @@
## master #531 +/- ##
=======================================
Coverage 38.32% 38.32%
=======================================
Files 13 13
Lines 2839 2839
=======================================
Hits 1088 1088
Misses 1671 1671
Partials 80 80 Continue to review full report at Codecov.
|
/ok-to-test |
/retest |
1 similar comment
/retest |
@nitishSr: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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.
@nitishSr +1 on the refactoring, just added one question, else LGTM.
@@ -1,4 +1,4 @@ | |||
package e2e | |||
package e2e_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.
qq: Is the name change intentional or accidental ? The PR description does not mention about e2e_test
package #531 (comment)
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.
@shubham-pampattiwar yes that is intentional change. Just wanted to be more specific here, to reflect that its a test package (taken as reference from Ginkgo docs)
+1 |
"io/ioutil" | ||
) | ||
|
||
func ReadFile(path string) ([]byte, error) { |
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 think we can remove this? This function doesn't do anything. Can remove later after merge. Thoughts?
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 see elsewhere we are using os.ReadFile as well which seems to do the same thing.
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.
Looks like os.ReadFile is more direct over io/ioutil now... at least from reading https://pkg.go.dev/io/ioutil@go1.17.6#ReadFile
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.
@kaovilai I do think the same. I just kept it as it is, considering these changes were made by some earlier PR. I can definitely try to replace it altogether with os.ReadFile() if possible.
This PR tries to introduce new structure for organising test repo as per below structure -