Test/kgw #446

Yaiba · 2024-01-03T22:15:44Z

Add tests with KGW.

What I did:

pull kgw repo and build kgw docker image
add kgw integration tests on APIs and authn, they'll be skipped in short test mode
by default run all tests on self-hosted github runner
by default enable both acceptance test and integration test
a separated integration test 'TestLocalDevSetup' for local development, used by task dev:testnet:up:nb
combine all tests into one, reduce running time
minor fix on test issues

~~Tests are in integration tests, we probably should start using our own runners to run the tests, since integration tests will run longer and require more computing resources.~~. Right now our main CI workflow runs on our self-hosted github runner.

The tests are ready to review.

I'm testing CI workflow, do not merge before the tests pass.

Yaiba · 2024-01-08T17:47:28Z

internal/abci/abci.go

@@ -263,7 +264,7 @@ func (a *AbciApp) executeTx(ctx context.Context, rawTx []byte, logger *log.Logge
 			Code:    code.Uint32(),
 			GasUsed: gasUsed,
 			Events:  events,
-			Log:     "success",
+			Log:     "",


This I believe is useless and no logic is depended on it, not a breaking change.

jchappelow · 2024-01-08T19:02:32Z

test/integration/test-data/test_db.kf

@@ -121,7 +114,11 @@ action divide($numerator1, $numerator2, $denominator) public view {
    select $up AS upper_value, $down AS lower_value;
 }

-@kgw(authn='true')


kgw shouldn't auth owner actions?

No, an owner actoin is not necessary need to be authed.
But an onwer view action should(as user's identity is required) be, to be correctly used with KGW. I'll add this back to owner view action.

Just to clearfy, kgw annotation is not required for any action, kuneiform doesn't force it on any action.

But an onwer view action should(as user's identity is required) be, to be correctly used with KGW. I'll add this back to owner view action.

Ah, right right. I think that's what I was thinking -- non-transaction exec of owner view action.

Here I'm more about mark an action that require authn just for testing, not think this kf as a whole application. I should haha

Yaiba · 2024-01-08T19:26:34Z

As for the refactor on testing with better specification #392 (comment), I did a small iteration the result is not so good, I'll need more time on it.

Yaiba · 2024-01-08T20:16:56Z

I'll combine unit-test and integration-test as they share some same env setups.
It will reduce the running time, also stop running separated jobs(like before) if any error.

Yaiba · 2024-01-08T20:52:44Z

test/integration/kgw_test.go

+			defer helper.Teardown()
+
+			helper.RunDockerComposeWithServices(ctx, []string{"kgw"})
+			// this is boot up time issue when running in CI, we just wait for a while


This is actually due to the kgw upstream health check dealy, kwilteam/kgw#11 should fix thix

Yaiba · 2024-01-08T21:19:43Z

.github/workflows/ci.yaml

      - name: Setup Go
        uses: actions/setup-go@v4
        with:
-          go-version: '1.21'
+          go-version: '1.21.x'


Github runner is using the pre-installed go already, but it still takes 40s to run this step

Yaiba · 2024-01-08T21:44:47Z

This pr is good to go. No more email bombs on test failed 🙃
On average, tests will take less than 8 min, not ideal but it's a new start on our more throughly test workflow.

To get this merged, since I changed the job's name to 'test', github branch rule on status checks need to be disabled temporarily as it requires both unit-test and acceptance-test pass.

jchappelow · 2024-01-09T18:15:15Z

test/acceptance/docker-compose.yml

@@ -26,7 +26,7 @@ services:
      --root-dir=/app/kwil
      --log.level=${LOG_LEVEL:-info}
      --app.extension-endpoints=ext:50051
-      --app.admin-listen-addr=unix:///var/run/kwil/admin.sock
+      --app.admin-listen-addr=unix:///tmp/admin.sock


I thought we had the - /tmp:/var/run/kwil:rw mapping in volumes:target above for this. What was going wrong on the machine without this change?

'/var/run/kwil/admin.sock' just don't work locally on my pc, and I have to modify this manually everytime.
And I don't think we have to use this path for testing, as long as this path is inside the container?

jchappelow

Still working locally for me with a minor tweak (see comments).

jchappelow · 2024-01-09T18:30:13Z

Taskfile.yml

@@ -216,7 +216,7 @@ tasks:
      - go test ./... -tags=ext_test -count=1 -race

  test:it:
-    desc: Run integration tests
+    desc: Run integration tests ('short' mode)


Not an issue with this PR, but I was testing with go 1.22rc1 and I found that go mod vendor fails with:

go: 'go mod vendor' cannot be run in workspace mode. Run 'go work vendor' to vendor the workspace or set 'GOWORK=off' to exit workspace mode.

This is documented:
golang/go#60056

With the addition of go work vendor, the go mod vendor command errors if there is a go.work file. Two ways we can consider dealing with this so it works on dev machines where there is likely a go.mod as well as CI and cloud build systems:

GOWORK=off go mod vendor

go mod vendor || go work vendor

For when 1.22 is release.

Well it looks like if the vendor directory is intended for use inside the docker image (while building) then it would need to be created with GOWORK=off go mod vendor, not go work vendor.
I think we should make this change to Taskfile.yml's vendor task definition.

yeah seems so. It's also a bit surprise bc we should not have go.work file anywhere for this workflow 🤔

jchappelow · 2024-01-09T21:37:35Z

.github/workflows/ci.yaml

@@ -85,7 +85,8 @@ jobs:
      - name: Generate go vendor
        #for faster builds and private repos, need to run this after pb:compile:v1
        run: |
-          task vendor
+          rm -rf vendor
+          GOWORK=off go mod vendor


The problem was on my local dev machine where I use task vendor. Can we change the Taskfile instead?

Got it, I used go mod vendor || go work vendor so current workflow locally won't break.

jchappelow

LGTM. Merge away assuming CI passes still.

Yaiba · 2024-01-09T22:56:24Z

Alright, I'll temporary disable github branch check rules and merge it.

* test kgw * go 1.22 compatiable `go mod vendor`

Yaiba self-assigned this Jan 3, 2024

Yaiba force-pushed the test/kgw branch 5 times, most recently from f098624 to fe4d8a7 Compare January 8, 2024 17:34

Yaiba changed the title ~~[WIP] Test/kgw~~ Test/kgw Jan 8, 2024

brennanjl approved these changes Jan 8, 2024

View reviewed changes

Yaiba force-pushed the test/kgw branch from fe4d8a7 to e4373a9 Compare January 8, 2024 17:41

Yaiba added the testing label Jan 8, 2024

Yaiba commented Jan 8, 2024

View reviewed changes

Yaiba force-pushed the test/kgw branch from e4373a9 to da56199 Compare January 8, 2024 19:02

jchappelow reviewed Jan 8, 2024

View reviewed changes

Yaiba force-pushed the test/kgw branch from 8dbbf53 to e11cc80 Compare January 8, 2024 19:52

Yaiba commented Jan 8, 2024

View reviewed changes

Yaiba force-pushed the test/kgw branch from ab3dcbe to 90623fd Compare January 8, 2024 21:36

Yaiba force-pushed the test/kgw branch from 90623fd to 58b89b1 Compare January 9, 2024 17:19

jchappelow self-requested a review January 9, 2024 18:10

jchappelow reviewed Jan 9, 2024

View reviewed changes

jchappelow approved these changes Jan 9, 2024

View reviewed changes

jchappelow reviewed Jan 9, 2024

View reviewed changes

Yaiba added 2 commits January 9, 2024 15:37

test kgw

7008107

disable go work when do go mod vendor

33782d2

Yaiba force-pushed the test/kgw branch from f127685 to 33782d2 Compare January 9, 2024 21:37

Yaiba added 2 commits January 9, 2024 15:44

go 1.22 compatiable go mod vendor

683fb64

set GOWORK=off with go mod vendor

fc35cf8

jchappelow approved these changes Jan 9, 2024

View reviewed changes

Yaiba merged commit 53c8500 into main Jan 9, 2024
1 check passed

Yaiba deleted the test/kgw branch January 9, 2024 22:57

brennanjl pushed a commit that referenced this pull request Feb 26, 2024

Test/kgw (#446)

e7cbbb6

* test kgw * go 1.22 compatiable `go mod vendor`

brennanjl pushed a commit that referenced this pull request Feb 26, 2024

Test/kgw (#446)

f716359

* test kgw * go 1.22 compatiable `go mod vendor`

jchappelow pushed a commit that referenced this pull request Feb 26, 2024

Test/kgw (#446)

21bcd18

* test kgw * go 1.22 compatiable `go mod vendor`

brennanjl pushed a commit that referenced this pull request Feb 26, 2024

Test/kgw (#446)

a117e68

* test kgw * go 1.22 compatiable `go mod vendor`

brennanjl pushed a commit that referenced this pull request Feb 26, 2024

Test/kgw (#446)

3d157a5

* test kgw * go 1.22 compatiable `go mod vendor`

brennanjl pushed a commit that referenced this pull request Feb 26, 2024

Test/kgw (#446)

16e15dd

* test kgw * go 1.22 compatiable `go mod vendor`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test/kgw #446

Test/kgw #446

Yaiba commented Jan 3, 2024 •

edited

Loading

Yaiba Jan 8, 2024

jchappelow Jan 8, 2024

Yaiba Jan 8, 2024

jchappelow Jan 8, 2024

Yaiba Jan 8, 2024

Yaiba commented Jan 8, 2024

Yaiba commented Jan 8, 2024

Yaiba Jan 8, 2024

Yaiba Jan 8, 2024

Yaiba commented Jan 8, 2024

jchappelow Jan 9, 2024

Yaiba Jan 9, 2024

jchappelow left a comment

jchappelow Jan 9, 2024

jchappelow Jan 9, 2024

Yaiba Jan 9, 2024

jchappelow Jan 9, 2024

Yaiba Jan 9, 2024

jchappelow left a comment

Yaiba commented Jan 9, 2024

Test/kgw #446

Test/kgw #446

Conversation

Yaiba commented Jan 3, 2024 • edited Loading

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

Yaiba commented Jan 8, 2024

Yaiba commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaiba commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow 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

jchappelow left a comment

Choose a reason for hiding this comment

Yaiba commented Jan 9, 2024

Yaiba commented Jan 3, 2024 •

edited

Loading