feat: Move integration tests to separate package #2111

sfc-gh-asawicki · 2023-10-10T21:58:25Z

This is the first PR to have SDK integration tests in a separate package. It is based on PoC: #1989.

It was essential to move the tests fast so that we could work on them in isolation and that there were not many strange conflicts to resolve. A few issues will be handled in subsequent PRs; they are described below.

Changes

(Almost) all integration tests moved to separate package
- setup I contained in the setup_integration_test.go file. It introduces two temporary methods, testClient and testContext, allowing easier test transition to the separate package. They should be inlined in subsequent changes.
- client_integration_test.go was left in the SDK package because this test is a bit different; it tests the client itself and not any resource in particular; therefore, its setup and behavior are different (I think it should be left there).
- This required prefixing some of the methods with sdk..
- Some unexported methods were needed in tests (and helpers), so they had to be exported somehow. To avoid big renames throughout the SDK package in this PR, they were copied/exported by delegating in the integration_test_imports.go file. The file contains a description; it will be one of the issues that must be handled in subsequent PRs.
- helper_test.go were almost completely moved to integration_tests/helpers.go file. Some methods are still needed in the SDK package (for sweepers and the aforementioned client_integration_test.go). It may change when we decide what to do with the sweepers.
- Two new precisely the same files, random.go, were introduced in both packages. We need to decide how to handle the need to have randomly generated content in both the sdk and integration_tests packages. A comment is left in the random.go file in the integration_tests package.

Next steps (for subsequent PRs) - summarized:

decide what is the right way to reuse client throughout the tests (currently accessor function testClient vs. itc.client vs. top-level client vs. something else?); the same with the testContext function
deal with the contents of integration_test_imports.go file
deal with two random.go files
decide what to do with sweepers
the tests were only moved, and so the common setup was not extracted:
- we can extract setting up database/schema to the testMain method
- we can merge separate functions in a single file to have less setup steps

Test Plan

unit tests (nothing should break)
integration tests (the essence of the tests was not changed, so they should pass)
acceptance tests (nothing should break)

References

feat: [PoC] Set up integration tests - with setup #1989

# Conflicts: # pkg/sdk/pipes_integration_test.go

Just move here. `sdk.` prefix is added automatically by IDE. WIP.

This allows us to not rename client in all integration tests. WIP.

This allows us to not rename ctx in all integration tests. WIP.

pkg/sdk/integration_tests/file_format_integration_test.go

pkg/sdk/integration_tests/sessions_integration_test.go

pkg/sdk/integration_tests/tasks_gen_integration_test.go

pkg/sdk/integration_tests/setup_integration_test.go

github-actions · 2023-10-10T22:14:45Z

Integration tests success for 4fd8e4fa3258e35a37e2be50b62c818ea2f8de5b

github-actions · 2023-10-10T22:16:20Z

Integration tests failure for 37b78520d7eb6d18e311bc04feb828aac5e20e90

github-actions · 2023-10-11T07:44:06Z

Integration tests success for f817192c25530d9f7156a78a219968adf4cf1a9d

sfc-gh-swinkler · 2023-10-11T08:15:58Z

pkg/sdk/integration_test_imports.go

+// This will be handled in subsequent PRs, so that the main difficulty (moving) is already merged.
+
+var (
+	ErrObjectNotExistOrAuthorized = errObjectNotExistOrAuthorized


i don't have an issue making errors public

As discussed I will do it in separate PR.

sfc-gh-swinkler · 2023-10-11T08:19:23Z

pkg/sdk/integration_test_imports.go

+)
+
+// ExecForTests is an exact copy of exec (that is unexported), that some integration tests/helpers were using
+func (c *Client) ExecForTests(ctx context.Context, sql string) (sql.Result, error) {


looks like it is only for tags / tables. both of which could be refactored to use the createTag() and createTable() `test helper functions. I don't like exporting this function, but it is okay for temporary

Yes, it's just temporary, I didn't want to do refactoring in this PR.

As discussed I will do it in separate PR.

sfc-gh-swinkler · 2023-10-11T08:24:34Z

pkg/sdk/integration_test_imports.go

+}
+
+// GetName is just an accessor to unexported name field
+func (r *CreateNetworkPolicyRequest) GetName() AccountObjectIdentifier {


Looks like we could have the helper functions which create network policy / other test objects also return the id. e.g. req, name := getDefaultNetworkPolicy(). So that way we do not need these getter functions. However, there is also no particular reason not to support getter functions. Perhaps we should add getter functions for all the request objects.

I will add it to our generator improvement doc, to generate at least a getter for the name for now.

As discussed I will do it in separate PR.

sfc-gh-swinkler · 2023-10-11T08:25:24Z

pkg/sdk/integration_test_imports.go

+}
+
+// GetAccountLocator is an accessor to unexported accountLocator, which is needed in some tests
+func (c *Client) GetAccountLocator() string {


i don't have a problem moving this to the config.go file. I think this plus the GetConfig() function are useful in general, not just for int tests.

As discussed I will do it in separate PR.

sfc-gh-swinkler · 2023-10-11T08:26:40Z

pkg/sdk/integration_test_imports.go

+	return c.config
+}
+
+// FindOne just delegates to our util findOne from SDK


i guess this begs the questions: should we move such helper functions into their own package, and have them all be exported? i don't see why not.

I think we can consider using internal packages for utils / helpers and other similar packages.
Here's more on internal packages - https://go.dev/doc/go1.4#internalpackages

Yes, that's what I would also recommend for this particular one. @sfc-gh-jcieslak do you agree?

Still, I would prefer to work on the contents of this file in subsequent PR to not inflate this PR any further (which would happen by using extracted util in existing places).

As discussed I will do it in separate PR.

pkg/sdk/integration_tests/accounts_integration_test.go

github-actions · 2023-10-11T12:52:38Z

Integration tests failure for 652a4b0df54514c678ef906d47000dc7418b662e

github-actions · 2023-10-11T13:11:12Z

Integration tests success for 652a4b0df54514c678ef906d47000dc7418b662e

github-actions · 2023-10-11T14:06:06Z

Integration tests failure for 7a6f781fbb6b2b49f2f028c6ba8a7df66eb19290

github-actions · 2023-10-11T14:28:19Z

Integration tests failure for 7a6f781fbb6b2b49f2f028c6ba8a7df66eb19290

github-actions · 2023-10-11T15:25:46Z

Integration tests failure for 7a6f781fbb6b2b49f2f028c6ba8a7df66eb19290

github-actions · 2023-10-11T15:50:21Z

Integration tests success for 7a6f781fbb6b2b49f2f028c6ba8a7df66eb19290

github-actions · 2023-10-11T16:20:51Z

Integration tests success for e9e60722013c629a8a5996c9490401a74f611305

sfc-gh-asawicki added 20 commits October 10, 2023 13:20

[PoC] Set up integration tests - with setup

0adbd9c

# Conflicts: # pkg/sdk/pipes_integration_test.go

Move all remaining integration tests

3b81572

Just move here. `sdk.` prefix is added automatically by IDE. WIP.

Introduce temporary testClient function

87352f5

This allows us to not rename client in all integration tests. WIP.

Introduce temporary testContext function

de77392

This allows us to not rename ctx in all integration tests. WIP.

Initialize new sdk client for close test

88a47ac

Fix imports ordering

00f2f16

Reintroduce context.Background() for helpers

c31cfbb

Move helpers to integration test package part 1 (WIP)

0b2ec2f

Use random helper functions from integration tests package

ace83f5

Use helper functions from integration tests package (WIP)

3aff352

Use helper functions from integration tests package part 2 (WIP)

9dd31dc

Use helper functions from integration tests package part 3 (WIP)

1e9ef9e

Export utility methods from sdk

16de49f

Use temporarily exported error

c59133b

Use helper functions from integration tests package part 4 (WIP)

26d29ce

Use helper functions from integration tests package part 5 (WIP)

d77bd65

Use helper functions from integration tests package part 6 (WIP)

9ab98e2

Use helper functions from integration tests package part 7 (WIP)

2add145

Prune old helper

1cf3c53

Fix last export problems

37b7852

github-actions bot reviewed Oct 10, 2023

View reviewed changes

sfc-gh-asawicki mentioned this pull request Oct 10, 2023

feat: [PoC] Set up integration tests - with setup #1989

Closed

1 task

Fix linters

4fd8e4f

Add explanatory comments

f817192

sfc-gh-asawicki marked this pull request as ready for review October 11, 2023 07:54

sfc-gh-swinkler requested changes Oct 11, 2023

View reviewed changes

sfc-gh-jcieslak previously approved these changes Oct 11, 2023

View reviewed changes

Rename package

652a4b0

sfc-gh-asawicki requested review from sfc-gh-swinkler and sfc-gh-jcieslak October 11, 2023 12:29

sfc-gh-asawicki dismissed sfc-gh-jcieslak’s stale review via 652a4b0 October 11, 2023 12:34

sfc-gh-jcieslak approved these changes Oct 11, 2023

View reviewed changes

Merge branch 'main' into move-integration-tests-to-separate-package

7a6f781

Merge branch 'main' into move-integration-tests-to-separate-package

e9e6072

sfc-gh-asawicki merged commit 2755589 into main Oct 11, 2023
8 checks passed

sfc-gh-asawicki deleted the move-integration-tests-to-separate-package branch October 11, 2023 16:23

snowflake-release-please bot mentioned this pull request Oct 11, 2023

chore(main): release 0.74.0 #2114

Merged

This was referenced Oct 11, 2023

chore: Adjust integration tests after moving to separate package #2115

Merged

chore: Integration tests setup db once #2118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Move integration tests to separate package #2111

feat: Move integration tests to separate package #2111

sfc-gh-asawicki commented Oct 10, 2023 •

edited

github-actions bot commented Oct 10, 2023

github-actions bot commented Oct 10, 2023

github-actions bot commented Oct 11, 2023

sfc-gh-swinkler Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

sfc-gh-swinkler Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

sfc-gh-swinkler Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

sfc-gh-swinkler Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

sfc-gh-swinkler Oct 11, 2023

sfc-gh-jcieslak Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

sfc-gh-asawicki Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

feat: Move integration tests to separate package #2111

feat: Move integration tests to separate package #2111

Conversation

sfc-gh-asawicki commented Oct 10, 2023 • edited

Changes

Next steps (for subsequent PRs) - summarized:

Test Plan

References

github-actions bot commented Oct 10, 2023

github-actions bot commented Oct 10, 2023

github-actions bot commented Oct 11, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

github-actions bot commented Oct 11, 2023

sfc-gh-asawicki commented Oct 10, 2023 •

edited