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

[audit]: client/keys: Test_runImportHexCmd could become flaky due to overzealous os.Removeall(t.TempDir()) instead of creating a fresh directory of its own #17854

Closed
1 of 4 tasks
odeke-em opened this issue Sep 25, 2023 · 2 comments · Fixed by #17855 or #17863
Assignees
Labels

Comments

@odeke-em
Copy link
Collaborator

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

Noticed in an audit that Test_runImportHexCmd added this code in v0.50.0-rc.0

+                       // Now add a temporary keybase
+                       kbHome := t.TempDir()
+                       kb, err := keyring.New(sdk.KeyringServiceName(), tc.keyringBackend, kbHome, nil, cdc)
+                       require.NoError(t, err)
+
+                       clientCtx := client.Context{}.
+                               WithKeyringDir(kbHome).
+                               WithKeyring(kb).
+                               WithInput(mockIn).
+                               WithCodec(cdc)
+                       ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
+
+                       t.Cleanup(cleanupKeys(t, kb, "keyname1"))
+
+                       defer func() {
+                               _ = os.RemoveAll(kbHome)
+                       }()

but a few problems:

  • (*testing.T).TempDir() is meant to be controlled by the framework itself
  • invoking kbHome := t.TempDir() should be fixed up to kbHome := filepath.Join(t.TempDir(), fmt.Sprintf("%x", tc.name)))
  • invoking defer func() { _ = os.RemoveAll(kbHome) }() deletes t.TempDir() but it should have gotten a fresh directory

We need to fix the above, otherwise the behavior will be undefined and it is trivial to fix compared to the problems that'll be caused.

Cosmos SDK Version

v0.50.0-rc.0

How to reproduce?

No response

@odeke-em odeke-em added the T:Bug label Sep 25, 2023
@odeke-em odeke-em self-assigned this Sep 25, 2023
odeke-em added a commit that referenced this issue Sep 25, 2023
…per test run

Fixes undefined behavior that could result in test flakes on various
environments by creating a unique directory per sub-test run of
Test_runImportHexCmd. (*testing).T.TempDir() must not be mucked
around with.

Fixes #17854
@JulianToledano
Copy link
Contributor

@odeke-em should we do the same with:

// Now add a temporary keybase
kbHome := t.TempDir()
kb, err := keyring.New(sdk.KeyringServiceName(), tc.keyringBackend, kbHome, nil, cdc)

@odeke-em
Copy link
Collaborator Author

Yes please @JulianToledano!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment