feat(cts): add client test for retry strategy #2633
Conversation
✅ Deploy Preview for api-clients-automation ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
|
Whooo ! So nice, so fast ! |
millotp
force-pushed
the
feat/retry-test
branch
from
83e3b98
to
f145701
Compare
clients/algoliasearch-client-scala/src/main/scala/algoliasearch/config/Host.scala
Outdated
Show resolved
Hide resolved
generators/src/main/java/com/algolia/codegen/cts/tests/TestsClient.java
Outdated
Show resolved
Hide resolved
clients/algoliasearch-client-java/algoliasearch/src/main/java/com/algolia/config/Host.java
Outdated
Show resolved
Hide resolved
| @@ -148,7 +154,16 @@ private function request( | |||
|
|
|||
| $retry = 1; | |||
| foreach ($hosts as $host) { | |||
| $uri = $uri->withHost($host); | |||
| if ($this->config->getHasFullHosts()) { | |||
There was a problem hiding this comment.
so IIUC the user has to explicitly call setFullHost to make that work right? if that's the case we should provide some inlined doc to at least explain what we expect
There was a problem hiding this comment.
Yeah, if they want to specify a different scheme/port, they need to use setFullHosts() , I guess this would require some explanation indeed ...
clients/algoliasearch-client-python/algoliasearch/http/hosts.py
Outdated
Show resolved
Hide resolved
generators/src/main/java/com/algolia/codegen/cts/tests/TestsClient.java
Outdated
Show resolved
Hide resolved
| @@ -13,6 +13,11 @@ export type Host = { | |||
| * The protocol of the host URL. | |||
| */ | |||
| protocol: 'http' | 'https'; | |||
There was a problem hiding this comment.
I'm not sure why we have a mix of scheme and protocol in the clients
There was a problem hiding this comment.
I guess it's from the legacy, I can rename everything if you want, but I have no preference
There was a problem hiding this comment.
idk the real definition of both I guess there's some overlap? to me scheme is http or https and protocol is how we transfer data, which is defined by the scheme?
idk idk up to you
There was a problem hiding this comment.
ahahah that's my state of mind
| public function setFullHosts($hosts) | ||
| { | ||
| $this->config['hasFullHosts'] = true; | ||
|
|
||
| return $this->setHosts($hosts); | ||
| } | ||
|
|
||
| public function getHasFullHosts() | ||
| { | ||
| return $this->config['hasFullHosts']; | ||
| } |
There was a problem hiding this comment.
not really a fan of this I kind of prefer the scheme port and host standalone definitions so there's no parsing etc. not sure what you prefer
There was a problem hiding this comment.
I hate this but flemme to do more php
There was a problem hiding this comment.
it can be improved in the future
There was a problem hiding this comment.
a small commit for a php master like you is what? 3 minutes?
There was a problem hiding this comment.
I know this is not perfect like that but unfortunately we tried to use the same methotology as in the other languages, and it's not as simple since we reuse the Retry Strategy of the current PHP client and it would require quite a lot of rework :(
There was a problem hiding this comment.
Let's merge this, this method will be only used for this test anyway.
|
(btw gg to everyone who contributed to this, it's huge!) |
🧭 What and Why
DI-1810
Add a retry strategy test, which works by creating 2 servers, the first one should timeout, and the second one returns something we can check against, then we make sure we had the correct response, and that the timeout server was called.
This ensures 3 things in the client:
Note: this only works in js for now, but we can add it everywhere