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: Adjust integration tests after moving to separate package #2115

Merged
merged 22 commits into from
Oct 12, 2023

Conversation

sfc-gh-asawicki
Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki commented Oct 11, 2023

This is the second PR to have SDK integration tests in a separate package. It is a continuation of #2111. It deals with the previous discussions and addresses the topics mentioned in the last PR.

Changes

  • deal with the contents of integration_test_imports.go file:
    • there is only one method left (with the comment)
    • errors exported from sdk
    • getter methods added to some request structs
    • validation method extracted
  • deal with two random.go files
    • random.go from the integration tests package is entirely removed
    • common randoms are moved to pkg/sdk/internal/random.go
    • SDK-specific random helpers are left with a comment in the sdk package
    • all randomX functions signatures were changed (t parameter was removed)
  • generator templates and code had to be adjusted to methods moved and/or renamed

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
  • 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 fewer 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

@github-actions
Copy link

Integration tests failure for d5a2ab72820865419a9a06fdb734b67548252bac

@github-actions
Copy link

Integration tests success for 8f2f53429bb52f42761e8f675da5c62f8e817204

@sfc-gh-asawicki sfc-gh-asawicki marked this pull request as ready for review October 12, 2023 07:30
pkg/sdk/database_role_test.go Outdated Show resolved Hide resolved
@@ -15,13 +17,13 @@ func TestDatabaseRoleCreate(t *testing.T) {

t.Run("validation: nil options", func(t *testing.T) {
var opts *createDatabaseRoleOptions = nil
assertOptsInvalidJoinedErrors(t, opts, errNilOptions)
assertOptsInvalidJoinedErrors(t, opts, ErrNilOptions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should errors also be move into the internal package? Especially since jan is going to do some work with that. i guess it work make package imports weird. like errors.ErrNilOptions, also golang doesn't like it when you overwrite the standard package names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would not go with moving them to internal because it may be a use case for SDK users to compare the error they got with the reference error to decide on the next steps of execution. That's why they are just exported, as we have discussed in #2111 (comment).

"github.com/stretchr/testify/require"
)

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

Choose a reason for hiding this comment

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

my friend, this should be UUID

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we have another discussion coming. :D I strongly prefer having abbreviations written in only the first letter capitalized, but you are right; for now, our codebase has them capitalized, so I will rename them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@github-actions
Copy link

Integration tests failure for 9ef8c9f475e4d0231c59a5674fee21dc50aca2ba

@github-actions
Copy link

Integration tests failure for 8ace52fd02b76cfde5e8ebed272f66bc5591cebf

@sfc-gh-asawicki sfc-gh-asawicki force-pushed the integration-tests-separate-package-chores branch from 8ace52f to 3d66a36 Compare October 12, 2023 12:02
@github-actions
Copy link

Integration tests failure for 3d66a3659ab0afb47e62a09389be22d278dadb9a

@github-actions
Copy link

Integration tests failure for 2c171d9820b4fee5a76a1a893a2bc2968013c6bf

pkg/sdk/poc/README.md Show resolved Hide resolved
pkg/sdk/random.go Show resolved Hide resolved
@github-actions
Copy link

Integration tests failure for 2c171d9820b4fee5a76a1a893a2bc2968013c6bf

@github-actions
Copy link

Integration tests success for 2c171d9820b4fee5a76a1a893a2bc2968013c6bf

@sfc-gh-asawicki sfc-gh-asawicki merged commit 3f528a8 into main Oct 12, 2023
8 checks passed
@sfc-gh-asawicki sfc-gh-asawicki deleted the integration-tests-separate-package-chores branch October 12, 2023 14:59
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.

None yet

3 participants