Redesign the config file #50
Conversation
|
Hey zhongyi-zhang! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
| "organization_id":"org-guid-here", | ||
| "space_id":"spcae-guid-here" | ||
| { | ||
| "operation": "deprovision", |
There was a problem hiding this comment.
According to the spec, I'm wondering if it's better to add plan_id field here.
There was a problem hiding this comment.
Let me clarify that, the config file no longer directly specifies the request body. The test framework is able to what the plan_id is currently and build the request body.
| "platform": "cloudfoundry", | ||
| "some_field": "some-contextual-data" | ||
| { | ||
| "operation": "bind", |
There was a problem hiding this comment.
Doesn't it need some optional fields such like context and bind_resource?
There was a problem hiding this comment.
The field is optional and the test framework doesn't add any case for it. So, I refine this in the new config file. Anytime we can add it back for new cases which require it. Does it make sense?
|
I think the framework is generally ok, and I noticed there are several |
|
@leonwanghui thanks for the review. Looking forward to the PRs after rebase. I'll help review. |
|
LGTM |
|
Thanks! Now one more approval is required... @mattmcneeney as you know about the redesign, could you approve it? BTW, can @norshtein, @leonwanghui, and me be added to the .pullapprove.yml? It would help the project move faster on the non-critical PRs (e.g. fixing typo) and engineering-only PRs (e.g. fixing logic which is not reflecting the purpose of a case) -- what we are pretty sure that they can be merged. |
|
@zhongyi-zhang I have added @norshtein and @leonwanghui as approvers. Thanks and have fun! |
|
@zhongyi-zhang I have added @norshtein and @leonwanghui as approvers. Thanks and have fun! |
4 similar comments
|
@zhongyi-zhang I have added @norshtein and @leonwanghui as approvers. Thanks and have fun! |
|
@zhongyi-zhang I have added @norshtein and @leonwanghui as approvers. Thanks and have fun! |
|
@zhongyi-zhang I have added @norshtein and @leonwanghui as approvers. Thanks and have fun! |
|
@zhongyi-zhang I have added @norshtein and @leonwanghui as approvers. Thanks and have fun! |
|
LGTM |
|
Hey zhongyi-zhang! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
|
@mattmcneeney thanks!! Seems the change doesn't have an effect on the existing PR... Just merge it this time. |
Solve #48.
Also do a refactor on the test framework.
This PR tries to do a minimal change on the test framework to adjust the config redesign. All the test cases are kept. Something that not looks good to me are just commented as
TODO.Currently only the mock server can be ensured to pass all the test cases. Will create subsequent issues and PRs to continue improving the checker.
The change in test.js is huge. Git diff works not very well on it. Actually most of the original logic are kept. The refactor only optimizes how to trigger them according to the new config file. Appreciate your reviews!