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

chore: add client tests #88

Merged
merged 34 commits into from
Jan 26, 2022
Merged

chore: add client tests #88

merged 34 commits into from
Jan 26, 2022

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Jan 13, 2022

🧭 What and Why

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

Suggestion is welcome for schema & namings, ..

Changes included:

This PR adds:

  • generator (to generate client tests)
  • a few sample tests

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.

That's a great POC! I still don't know if this is the path we should take as it might be harder to handle those cases in other languages, but I'd say let's keep going!

tests/common.ts Outdated Show resolved Hide resolved
tests/integration/clients/search/basic.json Outdated Show resolved Hide resolved
"object": "$index",
"path": ["saveObjects"],
"parameters": [
[
Copy link
Member

Choose a reason for hiding this comment

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

Maybe one object is enough :D

"expected": {
"match": [
{
"objectContaining": {
Copy link
Member

Choose a reason for hiding this comment

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

As said in the meeting, object is tied to JavaScript, we could find a wording that is less js-oriented for external contributors

Copy link
Member

Choose a reason for hiding this comment

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

(I'm saying that but I have no naming idea actually :D)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so I was about to go for just containing, but again, I remembered other matchers from jest, like stringContaining, arrayContaining, ... object is not a universal term across all the languages, but I don't think there's one. We need to pick something as a spec, and generate tests with properly modified names for the languages.

Anyway, we can still change objectContaining to something else like dictionaryContaining, or whatever. Feedback is welcome!

tests/integration/templates/javascript/step.mustache Outdated Show resolved Hide resolved
@eunjae-lee eunjae-lee changed the title chore: add integration test WIP chore: add client tests Jan 14, 2022
@eunjae-lee eunjae-lee requested review from a team and damcou and removed request for a team January 17, 2022 10:19
@@ -86,6 +86,7 @@
"packageName": "@algolia/client-analytics",
"hasRegionalHost": true,
"isDeHost": true,
"fallbackToUS": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only analytics client falls back to us region.

Comment on lines +29 to +38
{{#expectedError}}
await expect(new Promise((resolve, reject) => {
{{> step}}
if (actual instanceof Promise) {
actual.then(resolve).catch(reject);
} else {
resolve();
}
})).rejects.toThrow("{{{expectedError}}}")
{{/expectedError}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We promisify the return of the step and expects it to throw.

Comment on lines +55 to +57
if (actual instanceof Promise) {
actual = await actual;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return of the step can be a promise, so we resolve it and then move onto validating the result.

@eunjae-lee eunjae-lee requested a review from millotp January 25, 2022 10:43
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.

Almost good ! You need to fix some eslint issue in the main.ts file also

"parameters": {
"appId": "my-app-id",
"apiKey": "my-api-key",
"region": ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use // @ts-expect-error above the line to make the compilation work

"parameters": {
"appId": "my-app-id",
"apiKey": "my-api-key",
"region": ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove some code by removing the async await, for example:

describe('basic', () => {
  test('does not throw when region is not given', () => {
    expect(() => {
      // @ts-expect-error
      // eslint-disable-next-line no-new
      new AnalyticsApi('my-app-id', 'my-api-key', '', {
        requester: new EchoRequester(),
      });
    }).not.toThrow();
  });

  test('getAverageClickPosition throws without index', () => {
    const $client = createClient();
    // @ts-expect-error
    expect(() => $client.getClickPositions({})).toThrow(
      'Parameter `index` is required when calling `getClickPositions`.'
    );
  });
});

@millotp
Copy link
Collaborator

millotp commented Jan 25, 2022

Can you format the output file also ?

@eunjae-lee
Copy link
Contributor Author

Can you format the output file also ?

"generate": "yarn generate:methods:requets ${0:-javascript} ${1:-search} && yarn generate:client ${0:-javascript} ${1:-search} && yarn format ${0:-javascript}",

already doing

@eunjae-lee
Copy link
Contributor Author

Almost good ! You need to fix some eslint issue in the main.ts file also

yup, fixed here 69e2983

millotp
millotp previously approved these changes Jan 26, 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.

All good !

@eunjae-lee eunjae-lee merged commit 795035b into main Jan 26, 2022
@eunjae-lee eunjae-lee deleted the chore/integration branch January 26, 2022 11:08
shortcuts pushed a commit that referenced this pull request Apr 22, 2022
* chore: add client testing

* chore: skip when template is missing

* chore: skip when tests for client don't exist

* test region WIP

* fix tests

* remove unnecessary url from echo requester

* fix: update template to check region

* fix: add hasReigonalHost to generators

* fix: make regional optional in client testing

* fix: test asynchronous errors

* update tests

* fix: remove duplicated import statements

* chore: remove unused part

* chore: format cts output

* fix: remove createIndex

* chore: do not use post- script in package.json

* fix: type issues

* Update tests/CTS/client/templates/javascript/suite.mustache

Co-authored-by: Pierre Millot <[email protected]>

* chore: add eslint to tests

* chore: update output

* run java cts on the CI

* Revert "run java cts on the CI"

This reverts commit 7a0358d.

Co-authored-by: Pierre Millot <[email protected]>
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.

3 participants