feat: Add Testing suite for destination plugins by yevgenypats · Pull Request #369 · cloudquery/plugin-sdk

yevgenypats · 2022-11-07T16:35:28Z

Closes #348

Introduce testing suite for destination plugins as well as number of methods to including transformers and reverseTransformers to help testing suite be able to test "end-to-end".

This also adds more testing with a memoryDB plugin to test destination plugin and destination plugin testing suite

hermanschaaf

There's quite a bit to unpack here, so I haven't reviewed it in great detail, but noticed a few small testing-related bugs

plugins/destination_testing.go

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

erezrokah · 2022-11-07T17:35:51Z

Looks like the newly added tests on Windows are failing https://github.com/cloudquery/plugin-sdk/actions/runs/3412572062/jobs/5678256683

yevgenypats · 2022-11-07T18:22:28Z

Looks like the newly added tests on Windows are failing https://github.com/cloudquery/plugin-sdk/actions/runs/3412572062/jobs/5678256683

wow so weird. maybe something related to time. seems like it cannot delete stale data on windows or fails to do so.

yevgenypats · 2022-11-07T20:49:31Z

Ok I think found the bug on windows actions. seems like either it is "too fast" but more likely time.Now returns the same thing 🤷 per run.

erezrokah

I think the helper methods and memory plugin make sense.

I might be missing something with the test suites. Maybe we could make them more readable by having a clear structure of setup, act, assert, and splitting the suites (especially the overwrite one), so each one tests a single use case?

erezrokah · 2022-11-08T10:00:25Z

plugins/destination_testing.go

+	ctx := context.Background()
+	logger := getTestLogger(t)
+
+	t.Run("TestWriteOverwrite", func(t *testing.T) {


Maybe we should use a list of enums instead of multiple booleans? Then this should will be a for loop of the suites we want to run (instead of running, then skipping).

We can default to all if the list is nil

Tried a different approach now.

plugins/destination_testing.go

erezrokah · 2022-11-08T10:15:23Z

plugins/destination_testing.go

-	expectedResource := &schema.DestinationResource{
-		TableName: tables[0].Name,
-		Data:      TestData(),
+	if !s.skipTestOverWriteDeleteStale {


These lines don't impact the outcome of the test (you can remove them and TestDestinationPlugin passes

Not sure I understand. some plugins dont support DeleteStale

I mean you can remove the entire block (including DeleteStale) and the test still passes

But I want to run deletestale for plugins that support it so why remove it? or we are talking about a different block.

That is what's confusing for me, why do we need it if the test passes without it?

I removed it I was referring to a deletestale lateron where I check if it was deleted after that via read

erezrokah · 2022-11-08T10:17:31Z

plugins/destination_testing.go

+	sourceName := uuid.NewString()
+	resource := schema.DestinationResource{
+		TableName: table.Name,
+		Data:      testdata.TestData(),


I'm not that happy about different tests sharing the same data. I think it's better to have it duplicated than creating a dependency between the different tests, WDYT?

They dont share it. it creates new data every call to TestData

By share I mean if someone changes TestData it impacts all tests (i.e. I can add data to test something and it will break other tests accidentally)

It wont impact as it creates a new struct or we are talking about something else. Each test creates it's own testdata.

Oh you mean the function itself. yes but no one else is using it so let's keep it as is now given we didn't have tests before.

erezrokah · 2022-11-08T10:18:28Z

plugins/destination_testing.go

+		TableName: table.Name,
+		Data:      testdata.TestData(),
+	}
+	_ = resource2.Data[5].Set("00000000-0000-0000-0000-000000000007")


This might a bit hard to read. Maybe we can struct the test data in a way that it's clear what is its type?

Agree, there is no good way here. maybe we need some support from the SDK but I would leave this out for now as it turns a bit into an "ORM" given read abilities.

erezrokah · 2022-11-08T10:24:40Z

plugins/destination_testing.go

 }

-func DestinationPluginTestHelper(ctx context.Context, p *DestinationPlugin, logger zerolog.Logger, spec specs.Destination) error {
+func (s *DestinationPluginTestSuite) destinationPluginTestWriteOverwrite(ctx context.Context, p *DestinationPlugin, logger zerolog.Logger, spec specs.Destination) error {


Doesn't this test only OverWriteDeleteStale?

If I remove the code that handles skipTestOverWriteDeleteStale (both sections of it) the test fails.
I'm not sure how this tests only overwrite mode.

May I suggest splitting this suite to 2 distinct suites, 1 for overwrite mode and another for overwrite with delete stale?

I added a struct with options instead of with

erezrokah · 2022-11-08T10:28:55Z

plugins/destination_test.go

+
+func TestDestinationPlugin(t *testing.T) {
+	p := NewDestinationPlugin("test", "development", NewTestDestinationMemDBClient)
+	DestinationPluginTestSuiteRunner(t, p, specs.Destination{


I'm a bit confused by this. DestinationPluginTestSuiteRunner runs TestWriteOverwrite and TestWriteAppend, but we set the plugin write mode to WriteModeAppend.

I'd expect it so each suite will run on a specific plugin write mode

The testsuite will re-run Init. maybe I need to change it to get just the spec of the plugin instead of the destination.

yevgenypats · 2022-11-08T12:18:30Z

@erezrokah Thanks! Can you please take another look?

plugins/destination_testing.go

erezrokah · 2022-11-08T12:26:13Z

plugins/destination_testing.go

+
+	t.Run("TestWriteOverwrite", func(t *testing.T) {
+		t.Helper()
+		if suite.tests.Overwrite {


Shouldn't this be:

if suite.tests.Overwrite { t.Run("TestWriteOverwrite"... }

I think not because otherwise I cant call t.skip which will give indicator on what tests were skipped natively.

Ah, cool but then it should be if !suite.tests.Overwrite to skip when the condition is false?

erezrokah

The remaining comments are non blocking

🤖 I have created a release *beep* *boop* --- ## [1.1.0](v1.0.4...v1.1.0) (2022-11-08) ### Features * Add Testing suite for destination plugins ([#369](#369)) ([1a542b9](1a542b9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

This should go after this is merged - cloudquery/plugin-sdk#369

feat: Add Testing suite for destination plugins

85357ab

yevgenypats requested review from erezrokah and hermanschaaf November 7, 2022 16:35

Merge branch 'main' into feat/testing_suite_destination_plugins

cb1f243

github-actions bot added feat and removed feat labels Nov 7, 2022

fix lint

ac8fe19

yevgenypats mentioned this pull request Nov 7, 2022

fix: Make CQTypes to Schema and introduce versioning. #348

Closed

hermanschaaf suggested changes Nov 7, 2022

View reviewed changes

yevgenypats and others added 9 commits November 7, 2022 19:12

Update plugins/destination_testing.go

ad2957e

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Update plugins/destination_testing.go

2a17542

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Update plugins/destination_testing.go

6b33815

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Update plugins/destination_testing.go

9ad8829

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Update plugins/destination_testing.go

f68af8b

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Update plugins/destination_testing.go

887b4b1

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Update plugins/destination_testing.go

0203219

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Update plugins/destination_testing.go

5476ca1

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>

Merge branch 'main' into feat/testing_suite_destination_plugins

d26fefb

yevgenypats added 6 commits November 7, 2022 20:22

make MemDB public

9d6e37b

use memdb in serve testing

28881eb

add some more testing here

a8a3b07

debugging timenow on windows

8f7f86a

fix lint

a519789

fix windows on gh actions

e3a9b08

yevgenypats requested a review from hermanschaaf November 7, 2022 20:57

yevgenypats mentioned this pull request Nov 7, 2022

feat: Update destination to more managed SDK cloudquery/cloudquery#3842

Merged

yevgenypats added 2 commits November 7, 2022 23:30

minor fixes to TestSuite

d1cbe12

fix deadlock

499e2de

hermanschaaf approved these changes Nov 8, 2022

View reviewed changes

erezrokah requested changes Nov 8, 2022

View reviewed changes

address review

582c1d6

yevgenypats requested a review from erezrokah November 8, 2022 12:18

erezrokah reviewed Nov 8, 2022

View reviewed changes

plugins/destination_testing.go Outdated Show resolved Hide resolved

erezrokah reviewed Nov 8, 2022

View reviewed changes

address review

7e2a480

yevgenypats requested a review from erezrokah November 8, 2022 12:39

erezrokah approved these changes Nov 8, 2022

View reviewed changes

yevgenypats added the automerge label Nov 8, 2022

Merge branch 'main' into feat/testing_suite_destination_plugins

7769b9d

kodiakhq bot merged commit 1a542b9 into main Nov 8, 2022

kodiakhq bot deleted the feat/testing_suite_destination_plugins branch November 8, 2022 13:20

cq-bot mentioned this pull request Nov 8, 2022

chore(main): Release v1.1.0 #372

Merged

kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Nov 8, 2022

feat: Update destination to more managed SDK (#3842)

5e4fd5d

This should go after this is merged - cloudquery/plugin-sdk#369

SCKelemen pushed a commit to SCKelemen/cloudquery that referenced this pull request Nov 15, 2022

feat: Update destination to more managed SDK (cloudquery#3842)

cdc9280

This should go after this is merged - cloudquery/plugin-sdk#369

Conversation

yevgenypats commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hermanschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erezrokah commented Nov 7, 2022

Uh oh!

yevgenypats commented Nov 7, 2022

Uh oh!

yevgenypats commented Nov 7, 2022

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezrokah Nov 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yevgenypats commented Nov 8, 2022

Uh oh!

Uh oh!

erezrokah Nov 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

yevgenypats commented Nov 7, 2022 •

edited

Loading

erezrokah Nov 8, 2022 •

edited

Loading

erezrokah Nov 8, 2022 •

edited

Loading