cluster-api-provider-aws
cluster-api-provider-aws copied to clipboard
kubernetes-sigs
Improve testcase naming convention
/area testing
We have started following test naming convention for new test cases in CAPA, we can also do this for existing test cases to bring consistency.
We use this blog for reference. Sample PRs: https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3160 https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3091
/good-first-issue
@Madhur97: This request has been marked as suitable for new contributors.
Guidelines
Please ensure that the issue body includes answers to the following questions:
- Why are we solving this issue?
- To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
- Does this issue have zero to low barrier of entry?
- How can the assignee reach out to you for help?
For more details on the requirements of such an issue, please see here and ensure that they are met.
If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.
In response to this:
/area testing
We have started following test naming convention for new test cases in CAPA, we can also do this for existing test cases to bring consistency.
We use this blog for reference. Sample PRs: https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3160 https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3091
/good-first-issue
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.
@Madhur97: This issue is currently awaiting triage.
If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.
The triage/accepted label can be added by org members by writing /triage accepted in a comment.
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.
Have you considered the following, property-based convention, which is even easier to read and more informative? https://youtu.be/tWn8RA_DEic?t=3069
@Ankitasw thanks for assigning it to me :)
@Ankitasw @Madhur97 I was looking at the PR mentioned in the description for reference. I saw that the naming convention used is like "TestService_ServiceName" like "TestService_Delete". This naming convention has to be followed everywhere or I am heading in the wrong direction ?
And could you please point out to the directories I need to look for tests and change the test names using the convention. Thanks :)
Hey @VibhorChinda sorry for delayed response.
The naming convention you mentioned above is for functions. What we are trying to achieve here is each test case names should follow a naming convention. Till now, we have followed naming convention like this: Should_ExpectedBehavior_When_StateUnderTest as mentioned in the reference blog here.
Understood your point @Ankitasw. But there will be so many tests in whole codebase right. Should one PR be pointing towards all tests or should this PR contain child PRs targeting specific folders and files.
I am confused with the locations I need to make changes
This PR's scope is to make changes for all the test files. To ease it out, maybe you can start with one package in the code per PR. Wdyt?
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
Hey @sedefsavas no updates as such. Sorry for blocking this issue for so long.
Unassigning myself, so that anyone with some bandwidth can take this up.
Hello there, I have tried opening a small PR https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3667 to refactor the specs for the cloud/service/network package, please have a look and let me know if that the way in which we would like the changes to go forward for the other packages too.
Sticking with the camel case test naming convention is another option here. Core Cluster API is following that and few other providers as well. If there are places we don’t follow that convention we can fix those. I don't see a major readability benefit of the new convention but don't have a strong opinion, it is just another convention.
Thanks for the help, I updated the PR to reflect the test names having camel case, please have a look if that's how I would proceed for the other packages. https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3667
I think we deviated from what was expected here. We were talking about the testcase naming conventions, not function naming convention. As mentioned in the description of the issue, we would use any of the naming convention listed here and would be consistent around whole repo.
apologies for being inactive on the PR, please feel free to pick this issue up again.
Hi @Ankitasw @sedefsavas , I have raised a PR to fix all the test cases naming to camel case, could you please have a look #3966
@Madhur97 @fazpeerbaksh Is this issue resolved? If not, I would like to work on this!
/assign
Since there is no PR on this issue since a long time I will assign it to myself and will work on it. /assign
