feat(go): CTS implementation for go client #1509

mehmetaligok · 2023-05-02T10:06:20Z

🧭 What and Why

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

The CTS suite for go client implemented.

Changes included:

Test templates created for the generation.
- When creating tests, json unmarshaller is used to parse requests. Initially, I tried to create the request the regular way but couldn't do it properly. The main issues were mostly about different "oneof" types and some very detailed requests. Unfortunately, generating these in Go with the mustache is not easy since mustache is all about "Logic-less" templates.
- Maybe we can revisit this later. We may need some improvements for CTS in general because creating CTS stuff is hard to manage and not straightforward.
extras field added to test json specs.
- It is added for skipForGo prop. It was needed for three specific test cases. The client was working okay but the test unmarshalling was not working properly because of the payload. No need to block this improvement for this.
- These extras can be also used for some other cases in the future because the only request may not be enough for all the language generations.
Added some improvements for client generation.
- UnmarshalJSON implemented for request struct to handle json parsing for the direct request.
- "OneOf" model unmarshaling updated to add an extra validation.

🧪 Test

The test should run with CI.

netlify · 2023-05-02T10:06:29Z

✅ Deploy Preview for api-clients-automation ready!

Name	Link
🔨 Latest commit	`de5ce27`
🔍 Latest deploy log	https://app.netlify.com/sites/api-clients-automation/deploys/6454ceccd117a6000924c41c
😎 Deploy Preview	https://deploy-preview-1509--api-clients-automation.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

algolia-bot · 2023-05-02T10:07:09Z

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

millotp

Nice ! But it looks like the tests are not generated on the CI, I think it's an artifact issue, can you try to tweak .github/actions/restore-artifacts/action.yml file to include cts ?

millotp · 2023-05-02T15:52:42Z

scripts/cts/runCts.ts

@@ -22,6 +22,17 @@ async function runCtsOne(language: string): Promise<void> {
      );
      break;
    }
+    case 'go':
+      if (DOCKER) {


I don't think you need the distinction for docker, /usr/local/go/bin/go is already in the PATH

Unfortunately, it is in the PATH but somehow it is not working with js runtime. I can run with just go from the container but when I used it with the run function it is not working.

I couldn't quite figure out the reason last time. So I already opened a separate ticket to resolve this if possible.

millotp · 2023-05-02T15:54:42Z

tests/CTS/methods/requests/predict/updateSegment.json

@@ -55,6 +55,9 @@
          ]
        }
      }
+    },
+    "extras": {
+      "skipForGo": true


do you have an idea why the payload doesn't work here ? is it because of oneOf ?

Yes mainly because of "oneof". The contents of "oneof"s are very similar for this request. Since I used the json unmarshalling, the expected request body is not matching completely. It is unmarshalling it to a different "oneof" because it is matching with more than one statement.

When I tried to create the request with a constructor it worked without an issue, I couldn't make it work for the tests. I have opened another ticket to improve some of the CTS workflow for development. Maybe that can ease the development so we can take a look at this later on.

mehmetaligok · 2023-05-03T07:36:44Z

Nice ! But it looks like the tests are not generated on the CI, I think it's an artifact issue, can you try to tweak .github/actions/restore-artifacts/action.yml file to include cts ?

@millotp Oh, I tried on local but I haven't noticed the test code does not exist on the generated branch. It is my bad. Let me take a look and update here. Thanks for the heads up.

mehmetaligok · 2023-05-03T11:02:40Z

@millotp push problem for the generated branch should be resolved. It is ready to be reviewed again. I also do some extra tests for negative and positive CI scenarios to ensure the cts run is working properly for the CI. I believe everything is in place right now.

millotp

looks great GG ! Huge PR

millotp · 2023-05-03T15:09:35Z

templates/go/tests/requests/requests.mustache

+}
+
+{{#blocksRequests}}
+func Test{{#lambda.titlecase}}{{clientPrefix}}{{/lambda.titlecase}}_{{#lambda.titlecase}}{{operationId}}{{/lambda.titlecase}}(t *testing.T) {


I know the execution time is already very fast, but can you add t.Parallel() here and in the for loop pls ?

since currently, we are using only an echo requester adding t.Parallel() is bringing flakiness to the tests. Some tests start randomly failing.

We may need to add some complexity to test cases to handle this, I guess we can skip this at this phase.

no worries the test runs very fast already

millotp · 2023-05-03T15:14:36Z

templates/go/tests/requests/requests.mustache

+
+      {{#request.body}}
+      ja := jsonassert.New(t)
+      ja.Assertf(*echo.body, `{{{request.body}}}`)


is it possible to assert the query params too ? if they are returned from the echo requester

Thanks for noticing. While adding query param and header assertions I noticed a few problems regarding the custom requests. They are also fixed with 402d69a and all ready to be reviewed.

aallam · 2023-05-04T11:59:32Z

Nice work! and as you mentioned, I am for revisiting the CTS specs for easier tests generation! Well done! 🚀

… and tests

…plementation # Conflicts: # generators/src/main/java/com/algolia/codegen/cts/manager/CTSManagerFactory.java # scripts/cts/runCts.ts

millotp · 2023-05-09T08:19:47Z

templates/go/tests/requests/requests.mustache

+      {{#request.queryParameters}}
+        queryParams := map[string]string{}
+        require.NoError(t, json.Unmarshal([]byte(`{{{.}}}`), &queryParams))
+        for k, v := range queryParams {


I don't remember what we do in other languages, but it might be worth to do a strict comparaison here, and check that the number of query params is the same in queryParams and in echo.query

oh, I miss this before merging. But I noted this to check out.

millotp

looks amazing ! somehow kotlin shows up in the generation this time @aallam 🤔

feat(go): CTS implementation for go client

5d5753d

mehmetaligok added the Go label May 2, 2023

mehmetaligok self-assigned this May 2, 2023

Merge branch 'main' into feat/go-client-cts-implementation

e076ba3

mehmetaligok added 3 commits May 2, 2023 13:04

fix(docker): remove debug port

d436830

fix(go): update go command to run cts

ec98831

fix(go): update go command to run cts

f7bd2f4

mehmetaligok marked this pull request as ready for review May 2, 2023 11:47

mehmetaligok requested a review from a team as a code owner May 2, 2023 11:47

mehmetaligok requested review from aallam, Fluf22, millotp and shortcuts May 2, 2023 11:47

millotp reviewed May 2, 2023

View reviewed changes

algolia-bot and others added 4 commits May 3, 2023 11:52

fix(go): fix client name for go in CI

4470b18

test(go): try to fail cts run

90441ab

test(go): add common.go to artifacts

3ed5c93

test(go): fix common.go to artifacts path

97bca7f

mehmetaligok requested a review from millotp May 3, 2023 11:02

millotp reviewed May 3, 2023

View reviewed changes

Merge branch 'main' into feat/go-client-cts-implementation

a59f4c2

aallam previously approved these changes May 4, 2023

View reviewed changes

feat(go): update query param and headers implementation for go client…

402d69a

… and tests

mehmetaligok dismissed aallam’s stale review via 402d69a May 4, 2023 12:51

mehmetaligok requested a review from millotp May 4, 2023 14:53

mehmetaligok requested a review from aallam May 4, 2023 14:53

Merge remote-tracking branch 'origin/main' into feat/go-client-cts-im…

de5ce27

…plementation # Conflicts: # generators/src/main/java/com/algolia/codegen/cts/manager/CTSManagerFactory.java # scripts/cts/runCts.ts

millotp reviewed May 9, 2023

View reviewed changes

millotp approved these changes May 9, 2023

View reviewed changes

mehmetaligok merged commit 4fb9def into main May 9, 2023

mehmetaligok deleted the feat/go-client-cts-implementation branch May 9, 2023 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(go): CTS implementation for go client #1509

feat(go): CTS implementation for go client #1509

mehmetaligok commented May 2, 2023

netlify bot commented May 2, 2023 •

edited

Loading

algolia-bot commented May 2, 2023 •

edited

Loading

millotp left a comment

millotp May 2, 2023

mehmetaligok May 3, 2023

millotp May 2, 2023

mehmetaligok May 3, 2023

mehmetaligok commented May 3, 2023

mehmetaligok commented May 3, 2023

millotp left a comment

millotp May 3, 2023

mehmetaligok May 3, 2023

millotp May 4, 2023

millotp May 3, 2023

mehmetaligok May 4, 2023

aallam commented May 4, 2023

millotp May 9, 2023

mehmetaligok May 9, 2023

millotp left a comment

feat(go): CTS implementation for go client #1509

feat(go): CTS implementation for go client #1509

Conversation

mehmetaligok commented May 2, 2023

🧭 What and Why

Changes included:

🧪 Test

netlify bot commented May 2, 2023 • edited Loading

✅ Deploy Preview for api-clients-automation ready!

algolia-bot commented May 2, 2023 • edited Loading

✗ The generated branch has been deleted.

millotp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehmetaligok commented May 3, 2023

mehmetaligok commented May 3, 2023

millotp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aallam commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

netlify bot commented May 2, 2023 •

edited

Loading

algolia-bot commented May 2, 2023 •

edited

Loading