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

Refactor test to use hcvault NewTestCluster #658

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

somtochiama
Copy link
Member

Fixes: #648
Signed-off-by: Somtochi Onyekwere [email protected]

resource, err := pool.Run("vault", testVaultVersion, []string{"VAULT_DEV_ROOT_TOKEN_ID=" + testVaultToken})
if err != nil {
logger.Fatalf("could not start resource: %s", err)
cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is a problem or not but IIUC we're re-using the cluster created here with the "artificial" testing.T context in tests with a different testing.T. Would this pose any problem w.r.t. log output or anything else?

Copy link
Member Author

@somtochiama somtochiama May 23, 2022

Choose a reason for hiding this comment

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

It seems to be a problem if there's an error in NewTestCluster and t.Fatal is called there. Specifically the tests hangs
I considered copying the whole file or creating a new test cluster for each test, but both seem overkill. Trying to see if there's a different way to structure the tests.

Copy link
Member

@souleb souleb left a comment

Choose a reason for hiding this comment

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

Awesome work @somtochiama 🥇

github.com/ory/dockertest v3.3.5+incompatible h1:iLLK6SQwIhcbrG783Dghaaa3WPzGc+4Emza6EbVUUGA=
github.com/ory/dockertest v3.3.5+incompatible/go.mod h1:1vX4m9wsvi00u5bseYwXaSnhNrne+V0E6LAcBILJdPs=
github.com/ory/dockertest/v3 v3.8.0 h1:i5b0cJCd801qw0cVQUOH6dSpI9fT3j5tdWu0jKu90ks=
Copy link
Member

Choose a reason for hiding this comment

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

why do we still have dockertest in the deps?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is also used here. I will replace/remove it accordingly

Copy link
Member Author

Choose a reason for hiding this comment

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

dockertest is used in the awskms test here.
I am not sure how we can rewrite this test to not use docker. @aryan9600 any ideas?

cfg := api.DefaultConfig()
cfg.Address = testVaultAddress
cli, err := api.NewClient(cfg)
cli.SetToken(testClient.Token())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cli.SetToken(testClient.Token())
cli.SetToken(testVaultToken)

// Wait until Vault is ready to serve requests
if err := pool.Retry(func() error {
if err := func() error {
cfg := api.DefaultConfig()
cfg.Address = testVaultAddress
cli, err := api.NewClient(cfg)
Copy link
Member

Choose a reason for hiding this comment

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

we should check the error before setting the token.

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.

HCVault: refactor test to avoid using docker
3 participants