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

fix(test): exclude lock file from storage files count #13343

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

keroro520
Copy link
Contributor

@keroro520 keroro520 commented Dec 12, 2024

The static_file tests assert the count of storage files. However, when the feature flag "reth-db/disable-lock" is enabled, the lock file is not created. This inconsistency affects the file count assertions in tests.

This PR modifies the storage files counting logic to exclude the lock file, ensuring consistent test behavior regardless of whether the "disable-lock" feature is enabled or not.


Reproduce the failed tests:

cargo nextest run -v --locked --workspace --features 'jemalloc-prof' -E 'kind(lib)' -E 'kind(bin)' -E 'kind(proc-macro)' test_tx_based_truncation test_header_truncation

# or simply
make test-unit

Part of the output:

--- TRY 3 STDERR:        reth-provider providers::static_file::tests::test_tx_based_truncation ---
thread 'providers::static_file::tests::test_tx_based_truncation' panicked at crates/storage/provider/src/providers/static_file/mod.rs:548:18:
called `Result::unwrap()` on an `Err` value: Test case 0: file count mismatch | got: 9 expected: 10)

Location:
    crates/storage/provider/src/providers/static_file/mod.rs:547:32
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The static_file tests assert the count of storage files. However, when the
feature flag "reth-db/disable-lock" is enabled, the lock file is not created.
This inconsistency affects the file count assertions in tests.

This PR modifies the storage files counting logic to exclude the lock file,
ensuring consistent test behavior regardless of whether the "disable-lock"
feature is enabled or not.
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

removing the +1 is a lot nicer.

makes sense

pending @joshieDo

@mattsse mattsse added the C-test A change that impacts how or what we test label Dec 12, 2024
@mattsse mattsse enabled auto-merge December 12, 2024 17:23
auto-merge was automatically disabled December 13, 2024 04:31

Head branch was pushed to by a user without write access

@keroro520 keroro520 force-pushed the fix-test-count-files-lock-file branch from 450279b to e7db8e5 Compare December 13, 2024 04:31
@keroro520
Copy link
Contributor Author

@mattsse @joshieDo e7db8e5 makes some fixes. PTAL

@mattsse mattsse added this pull request to the merge queue Dec 13, 2024
Merged via the queue into paradigmxyz:main with commit 233dc7d Dec 13, 2024
42 checks passed
emhane pushed a commit to ethereum-optimism/op-reth that referenced this pull request Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-test A change that impacts how or what we test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants