-
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(cts): add tests for Predict #480
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 |
@@ -145,6 +145,7 @@ public Map<String, Object> postProcessSupportingFileData( | |||
bundle.put("clientPrefix", Utils.createClientName(client, language)); | |||
bundle.put("import", createImportName()); | |||
bundle.put("hasRegionalHost", hasRegionalHost); | |||
bundle.put("defaultRegion", client.equals("predict") ? "ew" : "us"); |
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's basic but should be enough for now, lmk if you want to change that to read the default field for example
@@ -101,6 +101,7 @@ export async function generateClientTests( | |||
client: `${createClientName(client, language)}Client`, | |||
blocks: modifyForMustache(testsBlocks), | |||
hasRegionalHost: hasRegionalHost ? true : undefined, | |||
defaultRegion: client === 'predict' ? 'ew' : 'us', |
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 as in the CTS
571111c
to
bdc5448
Compare
params: | ||
oneOf: | ||
- $ref: '#/modelsToRetrieve' | ||
- $ref: '#/typesToRetrieve' | ||
- $ref: '#/allParams' |
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.
This is the param that was wrong, we were accepting empty object while it should be either of them or both
@@ -11,7 +11,7 @@ const apiKey = | |||
const userId = 'user1'; | |||
|
|||
// Init client with appId and apiKey | |||
const client = predictClient(appId, apiKey); | |||
const client = predictClient(appId, apiKey, 'ew'); |
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.
why 'ew' when it's default?
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.
'ew' is the default value of the Predict spec servers
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.
Out of curiosity, do we know why they chose that region ?
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.
IIRC they have a staging env that is defaulted to that for now, it might change in the future
@@ -11,7 +11,7 @@ const apiKey = | |||
const userId = 'user1'; | |||
|
|||
// Init client with appId and apiKey | |||
const client = predictClient(appId, apiKey); | |||
const client = predictClient(appId, apiKey, 'ew'); |
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.
why 'ew' when it's default?
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 good to me :)
(How was it to write client tests?)
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.
Good job 🚀
] | ||
}, | ||
{ | ||
"testName": "does not throw when region is given", |
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.
Should we add a test with no "region" param passed at all ?
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 test above should do the same as it's an empty string
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.
Ah ok so the param is mandatory in any case ?
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.
In this client yes, but in the insights one for example, no region parameters will fallback to their fallback
host.
Quite easy actually :D |
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-464
Changes included:
This PR adds tests for the Predict client to the CTS, with some tweaking around this client.
us
, which is wrong in that case.🧪 Test
CI :D