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.

Sign up for GitHub

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

Jump to bottom

feat(cts): add tests for host, user agent, and timeouts #112

Merged
merged 20 commits into from
Feb 10, 2022
Merged

feat(cts): add tests for host, user agent, and timeouts #112

merged 20 commits into from
Feb 10, 2022

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Feb 1, 2022 •
edited
Loading

🧭 What and Why

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

Changes included:

  • Add new tests for host, user agent, and timeouts.

@eunjae-lee eunjae-lee changed the title Chore/add-client-tests chore: add tests for host, user agent, and timeouts Feb 1, 2022
@eunjae-lee eunjae-lee requested review from a team and shortcuts and removed request for a team February 1, 2022 14:41
shortcuts
shortcuts reviewed Feb 1, 2022
View reviewed changes
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

I like the idea!

Small things:

  • I think api.json file name makes more sense than basic.json, as we usually give the name of what we test
  • I'm not sure why we use nocheck for the test files

GG :D

@eunjae-lee eunjae-lee requested a review from shortcuts February 2, 2022 14:15
shortcuts
shortcuts reviewed Feb 7, 2022
View reviewed changes
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Solid! Some comments about missing tests, but lgtm overall!

@shortcuts
Copy link
Member

I'd say it's a feat(cts) more than a chore :D

@eunjae-lee eunjae-lee changed the title chore: add tests for host, user agent, and timeouts feat(cts): add tests for host, user agent, and timeouts Feb 9, 2022
@eunjae-lee
Copy link
Contributor Author

I'd say it's a feat(cts) more than a chore :D

yeah adding something to cts, so feat(cts) makes sense!

shortcuts
shortcuts reviewed Feb 9, 2022
View reviewed changes
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Looks good! Last questions :D

tests/CTS/client/recommend/api.json
]
},
{
"testName": "calls api with correct user agent",
Copy link
Member

Choose a reason for hiding this comment

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

(Not required for this PR) I don't know if it's the same for other clients, but we also send the client name in the UA in JS.

cc @millotp @damcou do you send it in PHP/Java?

We could maybe test Algolia%20for.*clientName or something similar

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me

shortcuts
shortcuts approved these changes Feb 9, 2022
View reviewed changes
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@eunjae-lee eunjae-lee merged commit 1188f69 into main Feb 10, 2022
@eunjae-lee eunjae-lee deleted the chore/add-client-tests branch February 10, 2022 10:26
shortcuts pushed a commit that referenced this pull request Apr 22, 2022
@eunjae-lee
feat(cts): add tests for host, user agent, and timeouts (#112)
c88d752 
* chore: add tests for host, user agent, and timeouts

* chore: rename test

* chore: add more tests

* fix: add host to EchoResponse

* chore: rename tests

* chore: fix host test and add region test

* Update suite.mustache

* chore: update generated tests

* update tests for region

* fix wrong timeouts

* wip

* update region tests
This was referenced May 24, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damcou damcou damcou left review comments

@shortcuts shortcuts shortcuts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eunjae-lee @shortcuts @damcou