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.

Sign up for GitHub

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

Jump to bottom

chore: Test support for object renaming #3130

Merged
merged 7 commits into from
Oct 22, 2024
Merged

chore: Test support for object renaming #3130

merged 7 commits into from
Oct 22, 2024

Conversation

sfc-gh-jcieslak
Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak commented Oct 11, 2024 •
edited
Loading

Changes

  • Added integration tests to show the inconsistency between ShowByID errors
  • Added acceptance tests to show different behaviors of objects after renaming higher-hierarchy objects
  • Added generated models for database and schema
  • Added new test command to Makefile that runs the tests (they're separated from the CI tests by env variable that has to be enabled to run the tests)

TODO

  • Pick the important tests that should be running with other acceptance tests
  • Transform shallow hierarchy tests into parametrized ones (similar to what we have right now in deep hierarchy test).

@github-actions GitHub Actions
Copy link

Integration tests failure for e5518f35125585992c125a4a682c246f939eaee2

@sfc-gh-jcieslak sfc-gh-jcieslak marked this pull request as ready for review October 14, 2024 11:05
@github-actions GitHub Actions
Copy link

Integration tests failure for 8456b5608238bda0a6465b22b3cf89506e98a844

sfc-gh-asawicki
sfc-gh-asawicki previously approved these changes Oct 16, 2024
View reviewed changes
pkg/sdk/testint/errors_integration_test.go Show resolved Hide resolved
pkg/resources/object_renaming_acceptance_test.go Show resolved Hide resolved
pkg/resources/object_renaming_acceptance_test.go
NoDependency DependencyType = "no_dependency"
)

func TestAcc_ShallowHierarchy_IsInConfig_RenamedInternally_WithImplicitDependency(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: do you think that setting up parametrized tests for the shallow hierarchy too would also help in readability?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, after I wrote the deeper hierarchy I saw how the tests are much more compact (and readable). I wanted to rewrite shallow ones, but on the other hand, I thought to leave them as they are just because they're already there and maybe it's better to leave them in separation and avoid if-ology in test setups (if possible). Don't have strong feelings for this one 😅 wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same feeling that the deeper ones are much more readable now. Usually, I am against complex setup in parametrized tests but this use case is a bit different, we care about the permutations so setting the tests this way would improve readability and make them more understandable. I also don't feel a need for a strong push to rewrite them now, maybe in one of the next PRs or something

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do it in the next pr

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping it open then

@github-actions GitHub Actions
Copy link

Integration tests failure for 0fdacc63ffc73c6a1a2f5e3644bfef6444114a3f

@sfc-gh-jcieslak sfc-gh-jcieslak merged commit d665419 into main Oct 22, 2024
9 of 10 checks passed
@sfc-gh-jcieslak sfc-gh-jcieslak deleted the test-support-for-object-renaming branch October 22, 2024 10:55
@snowflake-release-please snowflake-release-please bot mentioned this pull request Oct 22, 2024
Merged
This was referenced Oct 22, 2024
Merged
Merged
sfc-gh-jcieslak added a commit that referenced this pull request Nov 5, 2024
@sfc-gh-jcieslak
chore: Detect changes in lists and sets (#3147)
c3edb79 
Test cases for changes in lists and sets. From the given time, I only
went through essential cases that consisted of:
- Ignoring the order of list items after creation.
- Having the ability to update an item while ignoring the order.
For the testing, I created a test resource because currently, we don't
have anything to test more complex examples of certain resource
behaviors (temporary providers we create for custom diff testing are not
sufficient in this case). The resource is only added to the resource
list whenever a special env is set (we could remove it entirely and
leave some documentation in the resource file (and acc test file) on how
to prepare for the tests). The imitation of external storage was done by
creating a struct and its global instance (some of the things needed to
be exported since external changes tested in acceptance tests needed to
access those). Certain resource fields were researched to test different
approaches, each is described in the implementation file.

Also added an assert on lists/sets that is able to assert items in order
independent manner (it was needed for the tests of the proposals).

> Note: Only lists were tested, because there was no major issue with
them in current resources. For the lists the following issues were
addressed: #420, #753

## Next pr
- Apply (parameterized tests in object renaming test cases)
#3130 (comment)
sfc-gh-jcieslak added a commit that referenced this pull request Nov 5, 2024
@sfc-gh-jcieslak
chore: join object renaming tests into parameterized ones (#3154)
be13502 
## Changes
Follow-up for
#3130 (comment).

Tests joined from standalone tests to parameterized ones. Two tests were
left, because:
- First represents the non-deterministic behavior of Terraform and it
would be good to have it separated
- Second, is also unique and tests the correlation of config order to
execution order

Commented test cases are there intentionally because normally they were
skipped due to errors that cannot be asserted with terraform testing
framework.

**Question:** The "important" tests are still not separated from the
regular object renaming tests because they're currently failing in a way
they're not assertable by the terraform provider testing framework (they
will always fail). In this case, should we just leave it as is or just
mark the "important" tests with TODO and ticket number?
sfc-gh-jmichalak pushed a commit that referenced this pull request Nov 8, 2024
@snowflake-release-please
chore(main): release 0.98.0 (#3137)
003999e 
##
[0.98.0](v0.97.0...v0.98.0)
(2024-11-08)

Feature scope readiness for V1:
[link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD)
([Roadmap
reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#wrap-up-the-functional-scope)).
:exclamation: Migration guide: [v0.97.0 ->
v0.98.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0970--v0980)

### 🎉 What's new
- New resources:
- authentication_policy
([#3098](#3098)),
references
[#2880](#2880)
- external_volume
([#3106](#3106)),
partially references
[#2980](#2980)
- stream_on_directory_table
([#3129](#3129))
- stream_on_view
([#3150](#3150))
- primary_connection, secondary_connection
([#3162](#3162))
- secret_with_basic_authentication, secret_with_generic_string,
secret_with_oauth_authorization_code_grant,
secret_with_oauth_client_credentials
([#3110](#3110)),
([#3141](#3141))
- New data sources:
- connections
([#3155](#3155)),
([#3173](#3173))
- secrets
([#3131](#3131))
- Reworked:
- provider configuration hierarchy
([#3166](#3166)),
references
[#1881](#1881),
[#2145](#2145),
[#2925](#2925),
[#2983](#2983),
[#3104](#3104)
- provider configuration fields
([#3152](#3152))
streams data source
([#3151](#3151))
- SDK upgrades:
- Upgrade tag SDK
([#3126](#3126))
- Recreate streams when they are stale
([#3129](#3129))
### 🔧  Misc
- Add object renaming research summary
([#3172](#3172))
- Test support for object renaming
([#3130](#3130)),
([#3147](#3147)),
([#3154](#3154))
- Add tests to issue
[#3117](#3117)
([#3133](#3133))
- New roadmap entry
([#3158](#3158))
- Test more authentication methods
([#3178](#3178))
- Minor fixes
([#3174](#3174))
### 🐛  Bug fixes
- Apply various fixes
([#3176](#3176)),
this addresses BCR 2024_08, references
[#2717](#2717),
[#3005](#3005),
[#3125](#3125),
[#3127](#3127),
[#3153](#3153)
- Connection and secret data sources tests
([#3177](#3177))
- Fix grant import docs
([#3183](#3183)),
resolves
[#3179](https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/3179)
- Fix user resource import
([#3181](#3181))
- Handle external type changes in stream resources
([#3164](#3164))
- Do not use OR REPLACE on initial creation in resources with
copy_grants
([#3129](#3129))
- Address issue
[#2201](#2201)
by introducing new stream resources

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-jmichalak sfc-gh-jmichalak sfc-gh-jmichalak approved these changes

@sfc-gh-asawicki sfc-gh-asawicki sfc-gh-asawicki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfc-gh-jcieslak @sfc-gh-asawicki @sfc-gh-jmichalak