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

storage: replace storage.TestStoreContext (and similar) with functions #1472

Closed
bdarnell opened this issue Jun 23, 2015 · 5 comments
Closed

storage: replace storage.TestStoreContext (and similar) with functions #1472

bdarnell opened this issue Jun 23, 2015 · 5 comments
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience help wanted Help is requested / needed by the one who filed the issue to fix it.
Milestone
Beta

Comments

@bdarnell
Copy link
Contributor

Mutable global contexts like storage.TestStoreContext can be accidentally mutated, as in #1471. It would be better to make these factory functions returning new unshared objects to avoid state accidentally leaking between tests.
@bdarnell bdarnell added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. help wanted Help is requested / needed by the one who filed the issue to fix it. labels Jun 23, 2015
@tamird
Copy link
Contributor

tamird commented Jun 23, 2015

Would constants work?

@bdarnell
Copy link
Contributor Author

No; composite types including structs cannot be const in go. golang/go#359

@petermattis petermattis changed the title Replace storage.TestStoreContext (and similar) with functions storage: replace storage.TestStoreContext (and similar) with functions Feb 12, 2016
@petermattis
Copy link
Collaborator

@bdarnell Do you still plan on doing this cleanup?

@tamird tamird added the E-easy Easy issue to tackle, requires little or no CockroachDB experience label Feb 12, 2016
@bdarnell
Copy link
Contributor Author

I'd still like for it to be done, but not enough to do it myself :) This would be a good task for a first-time contributor.

@tbg
Copy link
Member

tbg commented Feb 12, 2016

I also think this remains useful.

On Fri, Feb 12, 2016 at 2:11 PM Ben Darnell notifications@github.com
wrote:

I'd still like for it to be done, but not enough to do it myself :) This
would be a good task for a first-time contributor.

—
Reply to this email directly or view it on GitHub
#1472 (comment)
.

@petermattis petermattis modified the milestone: Beta Feb 14, 2016
petermattis added a commit to petermattis/cockroach that referenced this issue Mar 1, 2016
@petermattis
storage: change TestStoreContext from a var to a func
c5f4099 
Fixes cockroachdb#1472.
@petermattis petermattis mentioned this issue Mar 1, 2016
Merged
@blathers-crl blathers-crl bot mentioned this issue Jun 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience help wanted Help is requested / needed by the one who filed the issue to fix it.
Projects
None yet
Milestone
Beta
Development

No branches or pull requests
4 participants
@bdarnell @tamird @tbg @petermattis