Skip to content

test: svm_inspect_account helpers #5842

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

Merged
merged 1 commit into from
Apr 16, 2025

Conversation

2501babe
Copy link
Member

@2501babe 2501babe commented Apr 16, 2025

Problem

AccountLoader inspects accounts every time load_account() is called. the svm_inspect_account test asserts that an account was inspected a specific number of times, in a specific order, with specific states, rather than a looser pattern where it only checks eg "this was inspected as write at least once." based on several previous conversations, it would be undesirable to relax either of these conditions

this means any time we change how AccountLoader is used in svm, this test breaks. there are several past and future prs that cause accounts to be inspected several times per transaction:

  • program cache removed from account loader: program ids and loaders are inspected once more
  • simd83 balance collection: every account is inspected twice more
  • filter executable accounts fix: every account is inspected once more
  • simd186 program data size fixes: it will change again but i dont know to what yet

every time this happens i have to go through the test again and fix it

Summary of Changes

add some helpers so i can just increment an integer when account inspection frequency changes

if youd rather change the test to .find() the records it needs in the actual inspections, id be happy to approve that pr. changing AccountLoader to only inspect accounts it gets from accounts-db is not an option, because it may load and inspect an account as read-only and then later take it as write

(the original draft of AccountLoader had a mechanism to avoid re-inspecting accounts, but we collectively decided to remove this since inspection is supposed to signal "svm is looking at this account btw" and its up to the implementor of the trait [in this case Bank] to decide whether it cares or not, hence why multiple inspection happens in the first place)

@2501babe 2501babe self-assigned this Apr 16, 2025
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.0%. Comparing base (e7d3003) to head (c9e9a9d).
Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #5842     +/-   ##
=========================================
- Coverage    83.0%    83.0%   -0.1%     
=========================================
  Files         828      828             
  Lines      375623   375623             
=========================================
- Hits       311918   311915      -3     
- Misses      63705    63708      +3     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@2501babe 2501babe marked this pull request as ready for review April 16, 2025 08:05
@2501babe 2501babe requested a review from a team as a code owner April 16, 2025 08:05
@2501babe 2501babe requested a review from brooksprumo April 16, 2025 08:05
Copy link

@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.

:shipit:

This impl looks good to me. As long as the accounts_lt_hash tests keep passing, I'm happy :)

@2501babe 2501babe merged commit 7bb8e2d into anza-xyz:master Apr 16, 2025
30 checks passed
@2501babe 2501babe deleted the 20250416_insphelp branch April 16, 2025 17:04
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.

4 participants