Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Replace id generation in integration and acceptance tests part1 #2747

Merged
merged 23 commits into from
Apr 29, 2024

Conversation

sfc-gh-asawicki
Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki commented Apr 25, 2024

In order to move to the unified ID generation (connected with the changes in #2737) to stabilize parallel builds, the following changes were introduced inside this PR:

  • IdsGenerator was introduced in TestClient. It allows us to get the ids of the default objects and build the new identifiers with the desired suffix (indicating the current build). Most of the helpers were moved to use this generator to get old and new ids. There are still TODOs left there (there are helpers that get the name of the object from the caller - we have to ensure that the names are generated the same way, or they have a good reason to be different). Ids generator is exposed so that both integration and acceptance tests can use it.
  • Changed the generation method of resource/datasource names to the exposed method described below in all the acceptance tests.
  • Fixed the random.go file in the SDK (so that this one is used only in the SDK unit tests - read the content of the file below, it had the long comment describing the problems with this file):
    • renamed to random_test.go
    • unexported all the methods
    • switch to appropriate methods in integration/acceptance tests
  • Next part of the change is to check all identifier creations throughout helpers/acceptance tests/integration tests and switch to a proper generator. This is not finished. The following methods were attacked (the rest will be tackled in the next PR):
    • NewExternalObjectIdentifier
    • NewExternalObjectIdentifierFromFullyQualifiedName
    • NewAccountIdentifier
    • NewAccountIdentifierFromAccountLocator (moved to ids generator)
    • NewAccountIdentifierFromFullyQualifiedName (2 usages left in business critical skipped tests)
    • NewAccountObjectIdentifier (testAccCheckDatabaseExistence left with todo, grantOwnershipToTheCurrentRole, testAccCheckSharePrivilegesRevoked, sdk.NewAccountObjectIdentifier("ACCOUNTADMIN"), sdk.NewAccountObjectIdentifier("ORGADMIN"), sdk.NewAccountObjectIdentifier("NOT_EXISTING_ACCOUNT"), sdk.NewAccountObjectIdentifier("does_not_exist"), current role/user, sdk.NewAccountObjectIdentifier(""))
    • NewAccountObjectIdentifierFromFullyQualifiedName
  • other smaller changes:
    • removed warehouse 2 in acceptance tests for now
    • fixed TestAcc_TaskOwnershipGrant_onFuture test

Left to be done regarding NewAccountObjectIdentifier:

  • alterMaterializedViewQueryExternally
  • checkDatabaseAndSchemaDataRetentionTime
  • checkDatabaseSchemaAndTableDataRetentionTime
  • unsafe_execute_acceptance_test.go
  • alterViewOwnershipExternally
  • check app_role_1
  • check hardcoded filter_by_role, stproc1, sp_pi, filterByRole, records
  • CreateDatabaseWithName (and other similar helpers)
  • check setup_test.go
  • ask @sfc-gh-jcieslak about sdk.NewAccountObjectIdentifier("S3_STORAGE_INTEGRATION"), sdk.NewAccountObjectIdentifier("GCP_STORAGE_INTEGRATION"), sdk.NewAccountObjectIdentifier("AZURE_STORAGE_INTEGRATION")
  • extract common helpers for sdk.NewAccountObjectIdentifier("does_not_exist" and similar

Copy link

Integration tests failure for 1c63c7df64bb6830486409a38f34026ed885c76b

Base automatically changed from cleanup-helpers-part6 to main April 25, 2024 12:21
@sfc-gh-asawicki sfc-gh-asawicki force-pushed the replace-id-generation-in-acceptance-tests-part1 branch from 1c63c7d to ebe54c3 Compare April 25, 2024 16:10
Copy link

Integration tests failure for ebe54c3b4b17747c3c9e24d407fa0845e1d6ea34

@sfc-gh-asawicki sfc-gh-asawicki changed the title chore: Replace id generation in acceptance tests part1 chore: Replace id generation in integration and acceptance tests part1 Apr 26, 2024
Copy link

Integration tests failure for 8a0b0df1c73af42709d3be0ed0c0e7c293fe307d

@sfc-gh-asawicki sfc-gh-asawicki marked this pull request as ready for review April 26, 2024 09:52
pkg/resources/database_role_acceptance_test.go Outdated Show resolved Hide resolved
pkg/sdk/testint/schemas_integration_test.go Outdated Show resolved Hide resolved
Copy link

Integration tests failure for e364be6ae736a176c31a7b4939409f3c242a4eb6

@sfc-gh-asawicki sfc-gh-asawicki merged commit cd4a914 into main Apr 29, 2024
8 of 9 checks passed
@sfc-gh-asawicki sfc-gh-asawicki deleted the replace-id-generation-in-acceptance-tests-part1 branch April 29, 2024 13:19
sfc-gh-asawicki added a commit that referenced this pull request May 20, 2024
Continuation of #2747 (topics connected with
`NewAccountObjectIdentifier`):
- `alterMaterializedViewQueryExternally` - moved to test helper
- `checkDatabaseAndSchemaDataRetentionTime` - id part reworked, method
left for assertions introduction
- `checkDatabaseSchemaAndTableDataRetentionTime` - id part reworked,
method left for assertions introduction
- `unsafe_execute_acceptance_test.go` - id part reworked, some methods
left for assertions introduction
- `alterViewOwnershipExternally` - moved to test helper
- check `app_role_1` - test role name extracted and parametrized in
tests
- check hardcoded `filter_by_role`, `stproc1`, `sp_pi`, `filterByRole`,
`records` - added to procedure rework
- ask @sfc-gh-jcieslak about
`sdk.NewAccountObjectIdentifier("S3_STORAGE_INTEGRATION")`,
`sdk.NewAccountObjectIdentifier("GCP_STORAGE_INTEGRATION")`,
`sdk.NewAccountObjectIdentifier("AZURE_STORAGE_INTEGRATION")` -
precreated integrations extracted
- extract common helpers for
`sdk.NewAccountObjectIdentifier("does_not_exist")` and similar -
extracted
- Fixed setup for multiple acceptance tests (to use random ids not
random strings)

Still left (connected with `NewAccountObjectIdentifier`):
- `CreateDatabaseWithName` (and other similar helpers)
- check `setup_test.go`

Additional changes:
- changed return type for `CurrentRole` and `CurrentUser` to ids
- Fixed ShowByID for application roles
- Added FAQ entry about possible identifier error
- Introduced `DatabaseId()` and `SchemaId()` helpers to identifiers and
convenience methods to use them
- Introduced some additional placeholders (like role ids for
ACCOUNTADMIN and ORGADMIN)
- Moved check destroy to a common place
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants