fix(cts): move the client test from ts to java APIC-467 #620
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
There was a problem hiding this comment.
Looks interesting to harmonize all the test generation, I'll let @eunjae-lee and @shortcuts validate the js part.
I let some questions regarding the format of the parameters in the tests. I thought we wanted to respect the number of parameters for languages like PHP and Java, and I think there are some differences, did I miss something ?
| "endAt": "2022-02-01T13:37:01Z" | ||
| } | ||
| ], | ||
| "parameters": { |
There was a problem hiding this comment.
I'm still a little confused with this example: how can I know that this is one parameter containing name, variant and endAt indexes here and not 3 distinct parameters ?
There was a problem hiding this comment.
This is handled by the generator (not in the PR but working in my upcoming one) where based on the name of the method, we look in the spec what the type should be and enhance the type accordingly, the same way it's done in the CTS. As this currently works for js java and php I assume it will work here as well.
There was a problem hiding this comment.
The convention here is to always have named parameter, that we can look up in the spec, and then based on the language we either provide the name parameters directly or just iterate on each one.
| ], | ||
| "parameters": {}, | ||
| "expected": { | ||
| "error": "Parameter `index` is required when calling `getClickPositions`." |
There was a problem hiding this comment.
I think for PHP, we can only know that the given number of parameters wasn't correct (0 instead of 1) but can we know that the name of this parameter was actually index ?
There was a problem hiding this comment.
In php if the parameter is not provided it will be equal to null right ? at least this is what's tested for in AnalyticsClient.php:203
| } | ||
| ], | ||
| "parameters": { | ||
| "events": [] |
There was a problem hiding this comment.
Same here, when I look at that example, I would expect having a method with only one events param expected, but the param is actually an insightEvents array with an events index.
There was a problem hiding this comment.
If you managed to make it work for the CTS it will be the same here, the parameters object is the same as in pushEvents.json, and the transformation through the generator is the same, so the template can reuse the same generateParams.mustache file
| "object": "$client", | ||
| "path": "getPersonalizationStrategy", | ||
| "parameters": [], | ||
| "parameters": {}, |
There was a problem hiding this comment.
So are we ok that {} means no params, and {"whatever" : []} means one param with an empty array, and we shouldn't encounter the case{[]}anymore ?
There was a problem hiding this comment.
yes that's basically it for typed languages, there is a distinction for js between parameters: {} and parameters: undefined but it doesn't affect php or java
| "parameters": [ | ||
| {} | ||
| ], | ||
| "parameters": {}, |
There was a problem hiding this comment.
Isn't there a mandatory param here ? I see no expected error.
There was a problem hiding this comment.
true, I don't really know how it will handle this case, I'm checking with the java
| "requests": [] | ||
| } | ||
| ], | ||
| "parameters": { |
There was a problem hiding this comment.
Same here, the param should be getRecommendationsParams with a requests index no ?
|
Some tests will have to change for the java and php to work, but this is not related to this PR, do you have want me to change something before merging @damcou ? |
3 participants
🧭 What and Why
🎟 JIRA Ticket: APIC-467
Move the client tests generation from ts to java, to be able to reuse the type enhancement code, which is mandatory for typed languages like java, and useful for php too.
Changes included:
hasRegionalHostfrom openapitools.json (can be derived from the spec)camelizeandcapitalizefor consistency with tsparametersbetween requests and client tests, to always be an array🧪 Test
CI