test(cli): Add cross env tests for `gen,sync` commands by erezrokah · Pull Request #1848 · cloudquery/cloudquery

erezrokah · 2022-09-18T13:31:35Z

Summary

Adds basic integration tests. The sync test is failing due to a bug in our release pipeline (not embedding versions) which I'll fix in a separate PR

Use the following steps to ensure your PR is ready to be reviewed

Read the contribution guidelines 🧑‍🎓
Test locally on your own infrastructure
Run go fmt to format your code 🖊
Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
Update or add tests 🧪
Ensure the status checks below are successful ✅

erezrokah · 2022-09-18T13:32:05Z

.github/workflows/test_build.yml

      - id: set-matrix
        run: ./scripts/resolve-modules.sh
-  test_build:
+  test-build:


Keep casing consistent

erezrokah · 2022-09-18T13:32:14Z

.github/workflows/test_build.yml

      - main
 env: 
  CGO_ENABLED: 0
-  CQ_NO_TELEMETRY: 1


I don't think we need this anymore

.github/workflows/test_integration.yml

cli/integration_tests/gen_test.go

cli/integration_tests/sync_test.go

erezrokah · 2022-09-18T15:34:11Z

OK so I found

cloudquery/cli/cmd/generate_test.go

Line 70 in 9e3ace7

func TestGenerate(t *testing.T) {

and

cloudquery/cli/cmd/sync_test.go

Line 9 in 9e3ace7

func TestSync(t *testing.T) {

so I'll merge the two.
The separation to integration tests is so we can run them on Windows/MacOS too

…#1856) #### Summary This PR fixes cloudquery/docs#216 which is still present in v2 and causing failures in #1848. Also somehow related to #908 If you try: 1. `gen source test` 2. `gen destination postgresql` 3. `gen sync .` you'll get the follow error: ``` Loading specs from directory: . Downloading plugin from: https://github.com/cloudquery/cloudquery/releases/download/plugins/source/test/development/test_darwin_arm64.zip to: .cq/plugins/github/cloudquery/test/development/cq-source-test_darwin_arm64.zip Error: failed to sync source test: failed to get source plugin client for test: failed to download plugin: bad status: 404 Not Found. downloading https://github.com/cloudquery/cloudquery/releases/download/plugins/source/test/development/test_darwin_arm64.zip exit status 1 ``` This is because the test plugin doesn't embed the version correctly when released (and probably others too), thus reports a `development` version which is embedded in the config: ```yml kind: "source" spec: # Name of the plugin. name: "test" # Version of the plugin to use. version: "development" ... ``` Then the CLI tries to download that version from GitHub and fails. Since we already know the version from GitHub I suggest we use that one instead of the gRPC call. For local/gRPC plugins the version is not important (at the moment). The version is already set in https://github.com/cloudquery/cloudquery/blob/7a83a65446119b5339f5f3a3759f7f160a3716b4/cli/internal/plugins/plugins.go#L137 so I only needed to pass in a pointer ---

erezrokah · 2022-09-18T16:16:25Z

OK so this PR improves on the current tests by:

Runs each test in a separate isolated directory so easy to add more scenarios
Runs the CLI via an external process closer to how the user runs it
Separate the integration tests from the unit tests and runs them on a multiple OS matrix

erezrokah · 2022-09-18T16:22:27Z

Looks like the CLI v2 doesn't work on Windows https://github.com/cloudquery/cloudquery/actions/runs/3077812862/jobs/4972939978#step:7:42

yevgenypats · 2022-09-18T18:38:26Z

OK so this PR improves on the current tests by:

Runs each test in a separate isolated directory so easy to add more scenarios

Runs the CLI via an external process closer to how the user runs it

Separate the integration tests from the unit tests and runs them on a multiple OS matrix

Im not a big fan of running this test as an external process as it makes testing good and bad test cases much hard as it will involve stdout output. I specifically built the cobra in such way that it should be possible to do so via NewRootCmd which is the main entry point so there shouldn't be any difference between this and an external process.

erezrokah · 2022-09-18T18:50:25Z

I specifically built the cobra in such way that it should be possible to do so via NewRootCmd which is the main entry point so there shouldn't be any difference between this and an external process.

Is there a way to run isolated tests in a temp directory and not in the CWD? That should allow us to reduce dependencies between tests and run tests in parallel.

Also I think testing stdout is not necessarily bad in the case of CLIs as that's what the user experiences. We can leverage snapshot testing on the CLI output to avoid the need to parse the it.
Probably better to discuss via a Zoom call :)

yevgenypats · 2022-09-18T21:42:18Z

I specifically built the cobra in such way that it should be possible to do so via NewRootCmd which is the main entry point so there shouldn't be any difference between this and an external process.

Is there a way to run isolated tests in a temp directory and not in the CWD? That should allow us to reduce dependencies between tests and run tests in parallel.

Also I think testing stdout is not necessarily bad in the case of CLIs as that's what the user experiences. We can leverage snapshot testing on the CLI output to avoid the need to parse the it. Probably better to discuss via a Zoom call :)

Yeah I think it should be possible to do in separate directories. Also, Im not a big fan of snapshot testing :) https://medium.com/@sapegin/whats-wrong-with-snapshot-tests-37fbe20dfe8e
but sure let's do zoom 📞

.github/workflows/test_unit.yml

cli/integration_tests/gen_test.go

erezrokah · 2022-09-19T08:07:29Z

Yeah I think it should be possible to do in separate directories. Also, Im not a big fan of snapshot testing :) https://medium.com/@sapegin/whats-wrong-with-snapshot-tests-37fbe20dfe8e

I'm familiar with the article and agree that in most cases (like testing Web apps with Jest) they don't provide much value.
I do think for CLIs they can provide benefits but lets discuss separately as it's out of scope for this PR and for v2 initial iteration. Opened a separate issue to discuss #1860

cli/cmd/generate_test.go

yevgenypats

Let's have a live discussion on the integration testing strategy?

yevgenypats · 2022-09-18T15:21:47Z

cli/cmd/doc_test.go

@@ -1,3 +1,5 @@
+//go:build unit


maybe do !integration so it will run with regular go test ./... ?

Also, why have integration and not integration? I think we can always run both - also for developers as those shouldn't take long. wdyt?

This code it outdated per @hermanschaaf's comment. We don't have separate unit tests and integration tests.
This PR introduces a separate workflow that runs only the CLI unit tests on Windows and MacOS (yet to be completed):
https://github.com/cloudquery/cloudquery/pull/1848/files#diff-0dbaaba16effff4896e9054770ab112bf49ae5b1e41a70915abafeef253b9ca7R31

yevgenypats

Let's discuss live but see comment here - #1860 (comment)

I don't think that we need to test/maintain output snapshot/predictability of a CLI

erezrokah · 2022-09-28T08:42:19Z

I don't think that we need to test/maintain output snapshot/predictability of a CLI

To clarify, this PR does not introduce snapshot testing. It adds a workflow to run exiting CLI tests on Windows and Mac and changes the CLI tests so they can be executed in a temporary directory and not the CWD of the tests

erezrokah · 2022-10-02T14:07:58Z

Replaced by #2204

github-actions bot added the cli label Sep 18, 2022

erezrokah changed the title ~~test(cli): Add integration tests for~~ test(cli): Add integration tests for gen,sync commands Sep 18, 2022

erezrokah commented Sep 18, 2022

View reviewed changes

.github/workflows/test_integration.yml Show resolved Hide resolved

erezrokah commented Sep 18, 2022

View reviewed changes

cli/integration_tests/gen_test.go Show resolved Hide resolved

erezrokah commented Sep 18, 2022

View reviewed changes

cli/integration_tests/sync_test.go Outdated Show resolved Hide resolved

erezrokah force-pushed the test/integration branch from c1a86a3 to 3562b70 Compare September 18, 2022 13:37

This was referenced Sep 18, 2022

fix(release): Embed correct modules version #1849

Merged

fix: Resolve plugin version from GitHub registry instead of gRPC call #1856

Merged

erezrokah force-pushed the test/integration branch from 611c391 to 50e9e9b Compare September 18, 2022 16:06

erezrokah marked this pull request as ready for review September 18, 2022 16:06

erezrokah requested review from hermanschaaf and yevgenypats as code owners September 18, 2022 16:06

hermanschaaf reviewed Sep 19, 2022

View reviewed changes

.github/workflows/test_unit.yml Outdated Show resolved Hide resolved

hermanschaaf reviewed Sep 19, 2022

View reviewed changes

cli/integration_tests/gen_test.go Show resolved Hide resolved

erezrokah mentioned this pull request Sep 19, 2022

Discussion: Testing strategies for the CLI #1860

Closed

erezrokah force-pushed the test/integration branch 4 times, most recently from 107fc80 to 009ab60 Compare September 19, 2022 09:48

hermanschaaf reviewed Sep 19, 2022

View reviewed changes

cli/cmd/generate_test.go Show resolved Hide resolved

hermanschaaf approved these changes Sep 19, 2022

View reviewed changes

erezrokah force-pushed the test/integration branch from 98ba12d to d1bd7aa Compare September 19, 2022 12:23

erezrokah added 5 commits September 19, 2022 15:52

test(cli): Add integration tests for the CLI

9836854

fix: Use correct binary path on Windows

70814a4

chore: Fix lint issues

c54fccd

chore: Fix workflows

5d612d4

chore: Cleanup test code

6481a8c

erezrokah force-pushed the test/integration branch 2 times, most recently from 5f6cc22 to b68f113 Compare September 19, 2022 13:35

erezrokah changed the title ~~test(cli): Add integration tests for gen,sync commands~~ test(cli): Add cross env tests for gen,sync commands Sep 19, 2022

erezrokah force-pushed the test/integration branch 5 times, most recently from 06a0bbf to b7f639d Compare September 19, 2022 15:37

chore: Setup postgres

0522d42

erezrokah force-pushed the test/integration branch from b7f639d to 0522d42 Compare September 19, 2022 15:46

chore: Setup postgres

93eaf6b

yevgenypats suggested changes Sep 22, 2022

View reviewed changes

yevgenypats suggested changes Sep 28, 2022

View reviewed changes

erezrokah closed this Oct 2, 2022

erezrokah deleted the test/integration branch October 2, 2022 14:08

Conversation

erezrokah commented Sep 18, 2022

Summary

Uh oh!

erezrokah Sep 18, 2022

Choose a reason for hiding this comment

Uh oh!

erezrokah Sep 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erezrokah commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah commented Sep 18, 2022

Uh oh!

yevgenypats commented Sep 18, 2022

Uh oh!

erezrokah commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yevgenypats commented Sep 18, 2022

Uh oh!

Uh oh!

Uh oh!

erezrokah commented Sep 19, 2022

Uh oh!

Uh oh!

yevgenypats left a comment

Choose a reason for hiding this comment

Uh oh!

yevgenypats Sep 18, 2022

Choose a reason for hiding this comment

Uh oh!

yevgenypats Sep 18, 2022

Choose a reason for hiding this comment

Uh oh!

erezrokah Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yevgenypats left a comment

Choose a reason for hiding this comment

Uh oh!

erezrokah commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah commented Oct 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erezrokah commented Sep 18, 2022 •

edited

Loading

erezrokah commented Sep 18, 2022 •

edited

Loading

erezrokah commented Sep 18, 2022 •

edited

Loading

erezrokah Sep 22, 2022 •

edited

Loading

erezrokah commented Sep 28, 2022 •

edited

Loading