-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(php): PHP tests #378
feat(php): PHP tests #378
Conversation
✅ Deploy Preview for api-clients-automation ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
# Conflicts: # scripts/cts/client/generate.ts # scripts/cts/utils.ts
✗ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice already! Will do an other pass once the CI is green
generators/src/main/java/com/algolia/codegen/cts/AlgoliaCtsGenerator.java
Outdated
Show resolved
Hide resolved
generators/src/main/java/com/algolia/codegen/cts/ParametersWithDataType.java
Outdated
Show resolved
Hide resolved
|
||
Map<String, Object> testOutput = createDefaultOutput(); | ||
testOutput.put("key", finalParamName); | ||
testOutput.put("parentSuffix", suffix - 1); | ||
testOutput.put("isFirstLevel", isFirstLevel); | ||
testOutput.put("hasGeneratedKey", finalParamName.matches("(.*)_[0-9]$")); |
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.
What is the hasGeneratedKey
referring to please?
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.
From what I understood, @millotp needs to generate keys to structure the data when there's no defined key in the test (for example here: https://github.com/algolia/api-clients-automation/blob/main/tests/CTS/methods/requests/recommend/getRecommendations.json#L34 where a requests_0
is generated by the custom generator. This is a problem for the PHP tests because the expected array structure is not the good one for the tests. That's why I added a boolean which checks if the key has been generated or not (if it's the case, I don't use it)
'body' => json_decode( | ||
"{\"events\":[{\"eventType\":\"click\",\"eventName\":\"Product Clicked\",\"index\":\"products\",\"userToken\":\"user-123456\",\"timestamp\":1641290601962,\"objectIDs\":[\"9780545139700\",\"9780439784542\"],\"queryID\":\"43b15df305339e827f0ac0bdc5ebcaa7\",\"positions\":[7,6]},{\"eventType\":\"view\",\"eventName\":\"Product Detail Page Viewed\",\"index\":\"products\",\"userToken\":\"user-123456\",\"timestamp\":1641290601962,\"objectIDs\":[\"9780545139700\",\"9780439784542\"]},{\"eventType\":\"conversion\",\"eventName\":\"Product Purchased\",\"index\":\"products\",\"userToken\":\"user-123456\",\"timestamp\":1641290601962,\"objectIDs\":[\"9780545139700\",\"9780439784542\"],\"queryID\":\"43b15df305339e827f0ac0bdc5ebcaa7\"}]}" | ||
), |
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 assume there's no prettier way to do it? :(
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.
At the beginning, I used simple quotes to embed the json, but for some paths, we use simple quotes as well, so as we already have the lambda.escapequotes
function available for the templates, I thought it was the easiest and more secured way to do it .
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.
Very cool ! just a few comments but it's working very well !
clients/algoliasearch-client-php/lib/Configuration/Configuration.php
Outdated
Show resolved
Hide resolved
generators/src/main/java/com/algolia/codegen/cts/AlgoliaCtsGenerator.java
Outdated
Show resolved
Hide resolved
@@ -24,7 +23,7 @@ async function runCtsOne(language: string, verbose: boolean): Promise<void> { | |||
{ verbose } | |||
); | |||
break; | |||
}*/ | |||
} |
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 cannot comment on line 22 but small question, do we need to run a command before to make sure that the phpunit
binary is there ?
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.
phpunit
is included in the composer.json
file so I don't know why the binary wouldn't be there. Do you have any specific potential issue in mind ?
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.
Okay but when do you run composer ?
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.
For example on the CI we run the CTS directly, we won't run composer before
@@ -27,6 +27,7 @@ export async function formatter( | |||
case 'php': | |||
cmd = `composer update --working-dir=clients/algoliasearch-client-php \ |
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'm getting some errors from this formatter: [error] No supported files were found in the directory: "tests/output/php/src".
. Do we need to add something to the docker image ? Or it as to do with the file naming (it generates them with lowercase) ?
|
||
Map<String, Object> testOutput = createDefaultOutput(); | ||
testOutput.put("key", finalParamName); | ||
testOutput.put("parentSuffix", suffix - 1); | ||
testOutput.put("isFirstLevel", isFirstLevel); |
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.
Maybe you can use {{^parent}}
instead of {{isFirstLevel}}
, because it should be null for root element
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.
If I use this, I get weird values like "indexName" => "[B@3e850122"
, that's why I added this boolean.
tests/CTS/methods/requests/templates/php/generateParams.mustache
Outdated
Show resolved
Hide resolved
tests/CTS/methods/requests/templates/php/generateParams.mustache
Outdated
Show resolved
Hide resolved
Can you remove the file |
clients/algoliasearch-client-php/lib/Configuration/Configuration.php
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
public function sendRequest( |
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 don't know much about phpunit, when is sendRequest called ?
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'm not a PHPUnit expert (nor PHP tests either) but the sendRequest()
method seems to be called each time a method is called from the client object (it has been implemented this way in the current PHP client). And instead of performing the true request, it stores it in an array of requests and return automatically a 200 response (the requests are tested afterwards in the assertRequests()
method).
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.
does that mean it doesn't go through the client code ?
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.
It goes through the code and stores the targeted api endpoint and the computed data sent (if any), the HTTP call is just not done.
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 really nice from my noob eye, I mostly have template comments
{ | ||
$client = $this->getClient(); | ||
|
||
{{#hasParameters}} | ||
|
||
$client->{{{method}}}( | ||
{{#parametersWithDataType}}{{> generateParams}}{{/parametersWithDataType}} | ||
); | ||
{{/hasParameters}} | ||
{{^hasParameters}} | ||
$client->{{{method}}}(); | ||
{{/hasParameters}} | ||
|
||
$this->assertRequests([ | ||
[ | ||
"path" => "{{{request.path}}}", | ||
"method" => "{{{request.method}}}", | ||
{{#request.data}} | ||
"body" => json_decode("{{#lambda.escapequotes}}{{{request.data}}}{{/lambda.escapequotes}}"), | ||
{{/request.data}} | ||
{{#request.searchParams}} | ||
"searchParams" => json_decode("{{#lambda.escapequotes}}{{{request.searchParams}}}{{/lambda.escapequotes}}"), | ||
{{/request.searchParams}} | ||
], | ||
]); | ||
} |
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.
We should remove some spaces here and inline mustache conditions, otherwise it puts newlines between every lines
e.g.
{ | |
$client = $this->getClient(); | |
{{#hasParameters}} | |
$client->{{{method}}}( | |
{{#parametersWithDataType}}{{> generateParams}}{{/parametersWithDataType}} | |
); | |
{{/hasParameters}} | |
{{^hasParameters}} | |
$client->{{{method}}}(); | |
{{/hasParameters}} | |
$this->assertRequests([ | |
[ | |
"path" => "{{{request.path}}}", | |
"method" => "{{{request.method}}}", | |
{{#request.data}} | |
"body" => json_decode("{{#lambda.escapequotes}}{{{request.data}}}{{/lambda.escapequotes}}"), | |
{{/request.data}} | |
{{#request.searchParams}} | |
"searchParams" => json_decode("{{#lambda.escapequotes}}{{{request.searchParams}}}{{/lambda.escapequotes}}"), | |
{{/request.searchParams}} | |
], | |
]); | |
} | |
{ | |
$client = $this->getClient(); | |
$client->{{^hasParameters}}{{{method}}}();{{/hasParameters}}{{#hasParameters}}{{{method}}}( | |
{{#parametersWithDataType}}{{> generateParams}}{{/parametersWithDataType}} | |
);{{/hasParameters}} | |
$this->assertRequests([ | |
[ | |
"path" => "{{{request.path}}}", | |
"method" => "{{{request.method}}}", | |
{{#request.data}}"body" => json_decode("{{#lambda.escapequotes}}{{{request.data}}}{{/lambda.escapequotes}}"),{{/request.data}} | |
{{#request.searchParams}}"searchParams" => json_decode("{{#lambda.escapequotes}}{{{request.searchParams}}}{{/lambda.escapequotes}}"),{{/request.searchParams}} | |
], | |
]); | |
} |
The linter should do the rest
{{#isFirstLevel}}{{#isString}}"{{{value}}}"{{/isString}} | ||
{{#isInteger}}{{{value}}}{{/isInteger}} | ||
{{#isDouble}}{{{value}}}{{/isDouble}} | ||
{{#isLong}}{{{value}}}{{/isLong}} | ||
{{#isBoolean}}{{{value}}}{{/isBoolean}} | ||
{{#isEnum}}"{{{value}}}"{{/isEnum}} | ||
{{#isArray}}[{{#value}}{{> generateParams}}{{/value}}]{{/isArray}} | ||
{{#isObject}}[{{#value}}{{> generateParams}}{{/value}}]{{/isObject}} | ||
{{#isFreeFormObject}}[{{#value}}{{> generateParams}}{{/value}}]{{/isFreeFormObject}} | ||
{{^-last}},{{/-last}} | ||
{{/isFirstLevel}} | ||
{{^isFirstLevel}} | ||
{{^hasGeneratedKey}}"{{{key}}}" => {{/hasGeneratedKey}} | ||
{{#isString}}"{{{value}}}",{{/isString}} | ||
{{#isInteger}}{{{value}}},{{/isInteger}} | ||
{{#isDouble}}{{{value}}},{{/isDouble}} | ||
{{#isLong}}{{{value}}},{{/isLong}} | ||
{{#isBoolean}}{{{value}}},{{/isBoolean}} | ||
{{#isEnum}}"{{{value}}}",{{/isEnum}} | ||
{{#isArray}}[{{#value}}{{> generateParams}}{{/value}}],{{/isArray}} | ||
{{#isObject}}[{{#value}}{{> generateParams}}{{/value}}],{{/isObject}} | ||
{{#isFreeFormObject}}[{{#value}}{{> generateParams}}{{/value}}],{{/isFreeFormObject}} | ||
{{/isFirstLevel}} |
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.
Same case here
{{#isFirstLevel}}{{#isString}}"{{{value}}}"{{/isString}} | |
{{#isInteger}}{{{value}}}{{/isInteger}} | |
{{#isDouble}}{{{value}}}{{/isDouble}} | |
{{#isLong}}{{{value}}}{{/isLong}} | |
{{#isBoolean}}{{{value}}}{{/isBoolean}} | |
{{#isEnum}}"{{{value}}}"{{/isEnum}} | |
{{#isArray}}[{{#value}}{{> generateParams}}{{/value}}]{{/isArray}} | |
{{#isObject}}[{{#value}}{{> generateParams}}{{/value}}]{{/isObject}} | |
{{#isFreeFormObject}}[{{#value}}{{> generateParams}}{{/value}}]{{/isFreeFormObject}} | |
{{^-last}},{{/-last}} | |
{{/isFirstLevel}} | |
{{^isFirstLevel}} | |
{{^hasGeneratedKey}}"{{{key}}}" => {{/hasGeneratedKey}} | |
{{#isString}}"{{{value}}}",{{/isString}} | |
{{#isInteger}}{{{value}}},{{/isInteger}} | |
{{#isDouble}}{{{value}}},{{/isDouble}} | |
{{#isLong}}{{{value}}},{{/isLong}} | |
{{#isBoolean}}{{{value}}},{{/isBoolean}} | |
{{#isEnum}}"{{{value}}}",{{/isEnum}} | |
{{#isArray}}[{{#value}}{{> generateParams}}{{/value}}],{{/isArray}} | |
{{#isObject}}[{{#value}}{{> generateParams}}{{/value}}],{{/isObject}} | |
{{#isFreeFormObject}}[{{#value}}{{> generateParams}}{{/value}}],{{/isFreeFormObject}} | |
{{/isFirstLevel}} | |
{{#isFirstLevel}}{{#isString}}"{{{value}}}"{{/isString}} | |
{{#isInteger}}{{{value}}}{{/isInteger}} | |
{{#isDouble}}{{{value}}}{{/isDouble}} | |
{{#isLong}}{{{value}}}{{/isLong}} | |
{{#isBoolean}}{{{value}}}{{/isBoolean}} | |
{{#isEnum}}"{{{value}}}"{{/isEnum}} | |
{{#isArray}}[{{#value}}{{> generateParams}}{{/value}}]{{/isArray}} | |
{{#isObject}}[{{#value}}{{> generateParams}}{{/value}}]{{/isObject}} | |
{{#isFreeFormObject}}[{{#value}}{{> generateParams}}{{/value}}]{{/isFreeFormObject}} | |
{{^-last}},{{/-last}}{{/isFirstLevel}}{{^isFirstLevel}} | |
{{^hasGeneratedKey}}"{{{key}}}" => {{/hasGeneratedKey}} | |
{{#isString}}"{{{value}}}",{{/isString}} | |
{{#isInteger}}{{{value}}},{{/isInteger}} | |
{{#isDouble}}{{{value}}},{{/isDouble}} | |
{{#isLong}}{{{value}}},{{/isLong}} | |
{{#isBoolean}}{{{value}}},{{/isBoolean}} | |
{{#isEnum}}"{{{value}}}",{{/isEnum}} | |
{{#isArray}}[{{#value}}{{> generateParams}}{{/value}}],{{/isArray}} | |
{{#isObject}}[{{#value}}{{> generateParams}}{{/value}}],{{/isObject}} | |
{{#isFreeFormObject}}[{{#value}}{{> generateParams}}{{/value}}],{{/isFreeFormObject}}{{/isFirstLevel}} |
Also, some conditions could be factorized (all they all used in PHP?)
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 actually tried those updates and for some reason, not only that it doesn't remove blank lines but it adds some :( . The linter doesn't remove the additional blank lines in PHP, TBH I don't know how to remove them, but I don't think it should be a blocker since it's only test classes.
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.
The code looks good, I'm surprised there is nothing to change in the client for it to pass, good job !
algolia/api-clients-automation#378 Co-authored-by: Damien Couchez <damien.couchez@gmail.com>
🧭 What and Why
🎟 JIRA Ticket: APIC-328
Changes included:
🧪 Test
yarn docker cts generate php
yarn docker cts run php