Skip to content
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(js): pass searchParams to EchoRequester, update CTS #70

Merged
merged 10 commits into from
Jan 10, 2022

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Jan 7, 2022

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-247

Changes included:

This PR aims at providing additional content to the EchoRequester.

Motivations

To successfully test GET requests and our clients, we will need extra parameters such as: searchParams, userAgent, timeouts, headers, etc.

We encounter this first limitation in #69, when we are not able to assert the search parameters.

🧪 Test

CI :D

@shortcuts shortcuts requested a review from damcou January 7, 2022 12:59
@shortcuts shortcuts self-assigned this Jan 7, 2022
@shortcuts
Copy link
Member Author

we could pass them as an object but the order might differ in the clients

Just realized we could also use the .toEqual(expect.objectContaining solution for the searchParams, but I wonder if other languages are able to do this

@shortcuts shortcuts force-pushed the feat/APIC-247/cts-search-parameters branch from 8a08566 to b0d3e23 Compare January 7, 2022 13:22
Base automatically changed from feat/APIC-241/cts-analytics to main January 7, 2022 14:51
@shortcuts
Copy link
Member Author

CI will not pass because of the Analytics client (others should not be impacted):

  • Paths are wrong (search is not a path parameter but query parameter, which breaks the encoding)
  • Dates needs to be casted since the type is not correct (they don't follow a Date structure, it's just a yyyy-mm-dd string)

Those changes will be applied in a future PR since it's not in the scope of this one

@shortcuts shortcuts force-pushed the feat/APIC-247/cts-search-parameters branch from 633070c to 6cc05ef Compare January 7, 2022 15:06
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good idea on the echo requester !

tests/generateCTS.ts Outdated Show resolved Hide resolved
tests/CTS/templates/javascript.mustache Outdated Show resolved Hide resolved
tests/CTS/templates/javascript.mustache Outdated Show resolved Hide resolved
@shortcuts shortcuts requested a review from millotp January 10, 2022 10:41
millotp
millotp previously approved these changes Jan 10, 2022
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of fixes !

@shortcuts shortcuts merged commit 817ba32 into main Jan 10, 2022
@shortcuts shortcuts deleted the feat/APIC-247/cts-search-parameters branch January 10, 2022 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants