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

Refactor assert in scan_snapshot_stores() #34162

Merged
merged 1 commit into from
Nov 18, 2023

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Nov 18, 2023

Problem

In #34118 we are upgrading Rust. The way Rust nightly displays panic messages has changed. This can cause should_panic tests to fail if they specify an expected message. We're seeing that in the accounts_db::tests::test_accountsdb_scan_snapshot_stores_illegal_range_xxx tests.

Here's what the error looks like:

test accounts_db::tests::test_accountsdb_scan_snapshot_stores_illegal_range_end - should panic ... FAILED

failures:

---- accounts_db::tests::test_accountsdb_scan_snapshot_stores_illegal_range_end stdout ----
thread 'accounts_db::tests::test_accountsdb_scan_snapshot_stores_illegal_range_end' panicked at accounts-db/src/accounts_db.rs:7644:9:
assertion failed: bin_range.start < bins && bin_range.end <= bins &&
    bin_range.start < bin_range.end
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: panic did not contain expected string
      panic message: `"assertion failed: bin_range.start < bins && bin_range.end <= bins &&\n    bin_range.start < bin_range.end"`,
 expected substring: `"bin_range.start < bins && bin_range.end <= bins &&\\n    bin_range.start < bin_range.end"`

Here's the build log: https://buildkite.com/solana-labs/solana/builds/104653#018be0ac-278d-4621-9fd4-629956045713

Since the newlines seem to be an issue, we can split up the assert so that the panic message doesn't have any newlines. There will be no functional difference.

Summary of Changes

Split the assert.

@brooksprumo brooksprumo self-assigned this Nov 18, 2023
@brooksprumo brooksprumo marked this pull request as ready for review November 18, 2023 15:07
Copy link

codecov bot commented Nov 18, 2023

Codecov Report

Merging #34162 (8889a5f) into master (07f1709) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34162     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      220120   220122      +2     
=========================================
- Hits       180367   180333     -34     
- Misses      39753    39789     +36     

Copy link
Contributor

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm. thanks!

@brooksprumo brooksprumo merged commit 574b8b5 into solana-labs:master Nov 18, 2023
32 checks passed
@brooksprumo brooksprumo deleted the rust/should_panic branch November 18, 2023 17:05
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.

2 participants