Changing tests to use u64 rather than bool for T during accounts index tests

[roryharr]

This is Pull Request 3 in a series to add support to upsert for obsolete accounts.

Problem

Accounts_index implements the traits (IndexValue, DiskIndexValue, IsCached, IsZeroLamport) required to create an index for multiple types (u64, bool, f64). The implementations for u64 and bool are the same. Some tests are better suited to using u64

Summary of Changes

• Any tests that relied on true/false to determine the correct entry were replaced with T u64

Fixes #

[roryharr]

Using *self rather than !*self required significantly more test changes (reversing true/false everywhere).

LMK

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.8%. Comparing base (fb1d632) to head (9d9e55b).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #7312     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         801      801             
  Lines      363429   363440     +11     
=========================================
- Hits       300993   300966     -27     
- Misses      62436    62474     +38     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[brooksprumo]

[..] none of the existing test types in accounts_index allow for testing of cached/uncached interactions.
[..]

• Changed the bool test type to return !*self for is_cached

I think we should add a new struct in the tests and don't overload what bool means for the cached-ness.

Something like this?

    struct CacheableIndexValue(bool);
    impl IndexValue for CacheableIndexValue {}
    impl DiskIndexValue for CacheableIndexValue {}
    impl IsCached for CacheableIndexValue {
        fn is_cached(&self) -> bool {
            self.0
        }
    }
    impl IsZeroLamport for CacheableIndexValue {
        fn is_zero_lamport(&self) -> bool {
            false
        }
    }

[roryharr]

[..] none of the existing test types in accounts_index allow for testing of cached/uncached interactions.
[..]

• Changed the bool test type to return !*self for is_cached

I think we should add a new struct in the tests and don't overload what bool means for the cached-ness.

Something like this?

    struct CacheableIndexValue(bool);
    impl IndexValue for CacheableIndexValue {}
    impl DiskIndexValue for CacheableIndexValue {}
    impl IsCached for CacheableIndexValue {
        fn is_cached(&self) -> bool {
            self.0
        }
    }
    impl IsZeroLamport for CacheableIndexValue {
        fn is_zero_lamport(&self) -> bool {
            false
        }
    }

If I were to do it that way, I would just create the new type and leave the existing bool alone. My thought was to differentiate bool and u64 as right now bool is only used to differentiate between two different entries, which u64 is more suited for.

Removing bool itself would involve modifications of over 100 tests, and would be a distraction from getting the obsolete account work completed.

An argument could definitely be made that not having analyzed every one of those tests, it is dangerous to rework bool in this way (ie unintended changes in test behaviour due to this). My thought is that worst case the behaviour isn't changing at all, best case we are getting free testing (ie tests that now fail) to think about.

[brooksprumo]

My thought was to differentiate bool and u64 as right now bool is only used to differentiate between two different entries, which u64 is more suited for.

I agree that u64 is better than bool here.

Removing bool itself would involve modifications of over 100 tests, and would be a distraction from getting the obsolete account work completed.

We could leave bool to avoid having to change the existing tests.

An argument could definitely be made that not having analyzed every one of those tests, it is dangerous to rework bool in this way (ie unintended changes in test behaviour due to this). My thought is that worst case the behaviour isn't changing at all, best case we are getting free testing (ie tests that now fail) to think about.

I agree reworking bool is probably fine here, but we're now overloading the value with the is-cached-ness in a way that is unexpected.

I keep coming back to that I'm not seeing any downsides to adding a new struct for explicit testing of cached index values.

[roryharr]

Member

Ok. repurposed this PR just to modify problematic u64/bool tests.

[brooksprumo]