Conversation
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
millotp
force-pushed
the
feat/helper-tests
branch
2 times, most recently
from
8a51cc3 to
04b4cd7
Compare
| "object": "$client", | ||
| "path": "generateSecuredApiKey", | ||
| "parameters": { | ||
| "parentAPIKey": "2640659426d5107b6e47d75db9cbaef8", |
There was a problem hiding this comment.
no worries this is our search api key, this means the generated key is usable for e2e
|
I was about to start adding those tests for PHP by hand, is it still useful or should I wait for this PR to be merged ? |
millotp
force-pushed
the
feat/helper-tests
branch
2 times, most recently
from
0ed2041 to
6f93939
Compare
millotp
force-pushed
the
feat/helper-tests
branch
from
2207ebe to
ec3f8c8
Compare
millotp
force-pushed
the
feat/helper-tests
branch
from
ec3f8c8 to
423e43b
Compare
| Object.prototype.toString.call(parameters[key]) === '[object Array]' | ||
| ? parameters[key].join(',') |
There was a problem hiding this comment.
I feel like this will break something
There was a problem hiding this comment.
it did break something, it wasn't working before with generateSecuredApiKey, but you agree with me that never ever do we have a query param that looks like a serialezed object right ? we only allow primitives and array to stringified
There was a problem hiding this comment.
Yup but IIRC this implem was exactly the same as the legacy client but since I never found the reason why it had an object case :/
There was a problem hiding this comment.
okay but we don't have any tests that expect that, and we don't have the same behavior in other languages, this was an outlier
There was a problem hiding this comment.
I can revert this if you want, but I think this is more correct
There was a problem hiding this comment.
I also think so but since it worked like that for years I think there's an edge case we are not aware of :/ we can leave it and see if there's complains
There was a problem hiding this comment.
ah bah it breaks the test because now it adds brackets to stringified array, thats why I had to put the .join(',')
There was a problem hiding this comment.
I mostly wonder how it could've been wrong for 4 years in the current JS client, maybe the engine supports this format?
Your initial solution is indeed correct to me, we can go with it and if user complain we fix it (we can also for example try it on the Crawler migration PoC too)
There was a problem hiding this comment.
sounds good, the PR should be fine then
| if isinstance(value, dict): | ||
| return dumps(value) | ||
| elif isinstance(value, list): | ||
| if isinstance(value, list): |
There was a problem hiding this comment.
does list validates dict? that's why you changed the order?
There was a problem hiding this comment.
same thing as for js, we never want to stringify a dict
This reverts commit b9ff92c.
4 participants
🧭 What and Why
DI-2464
Use the helper spec to generate tests, starting with the simplest: generateSecuredApiKey