Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

AcctIdx: tests can use index-buckets with an env var#21494

Merged
jeffwashington merged 1 commit intosolana-labs:masterfrom
jeffwashington:metrics16
Dec 9, 2021
Merged

AcctIdx: tests can use index-buckets with an env var#21494
jeffwashington merged 1 commit intosolana-labs:masterfrom
jeffwashington:metrics16

Conversation

@jeffwashington
Copy link
Copy Markdown
Contributor

@jeffwashington jeffwashington commented Nov 29, 2021

Problem

we have 2 code paths:

  1. using disk buckets
  2. not using disk buckets (in-mem only)

The 2 paths are connected (disk-buckets use in-mem for some operations). But, they are separate logic in multiple places. All tests that use accounts index ideally need to run ideally against both code paths.

Summary of Changes

default to in-mem
env var causes us to use disk buckets
Not:Randomly choose for each test to use disk buckets or not. Can we come up with a better solution other than replicating or parameterizing every test?

Fixes #

@jeffwashington
Copy link
Copy Markdown
Contributor Author

other options:

  1. plumbing with options all the way up through bank and maybe higher
  2. env var where we could run cargo test twice - once for disk buckets, once for in-mem. This could be just runtime or could be all tests in the whole 'coverage' step. What about local-cluster tests? validator? ledger-tool? stable?
  3. rework all accounts index tests to just run those both ways

brooksprumo
brooksprumo previously approved these changes Dec 1, 2021
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

I think that this change is incrementally better, so can go in as-is. Long term, I think option (3) of reworking the tests to explicitly test both version would be my vote.

I'd recommend actually opening a new issue with this, then marking it as "good first issue". I wouldn't be surprised if a community member did this work.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 1, 2021

Codecov Report

Merging #21494 (e3adcb5) into master (923720f) will decrease coverage by 0.2%.
The diff coverage is 80.0%.

@@            Coverage Diff            @@
##           master   #21494     +/-   ##
=========================================
- Coverage    81.6%    81.4%   -0.3%     
=========================================
  Files         511      511             
  Lines      143320   143324      +4     
=========================================
- Hits       116963   116666    -297     
- Misses      26357    26658    +301     

@t-nelson
Copy link
Copy Markdown
Contributor

t-nelson commented Dec 8, 2021

I think I prefer [2] as a stopgap to [3]. If we rename all of the tests to have the same prefix, we can add another CI step to do the run with the envvar set

@mergify mergify Bot dismissed brooksprumo’s stale review December 8, 2021 17:41

Pull request has been modified.

@jeffwashington jeffwashington changed the title AcctIdx: tests use non-index buckets 50% of time AcctIdx: tests can use index-buckets with an env var Dec 8, 2021
@jeffwashington jeffwashington marked this pull request as ready for review December 8, 2021 22:04
@jeffwashington jeffwashington merged commit 54862eb into solana-labs:master Dec 9, 2021
mergify Bot pushed a commit that referenced this pull request Dec 9, 2021
mergify Bot added a commit that referenced this pull request Dec 9, 2021
(cherry picked from commit 54862eb)

Co-authored-by: Jeff Washington (jwash) <wash678@gmail.com>
@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants