Skip to content

Conversation

JereSalo
Copy link
Contributor

@JereSalo JereSalo commented Oct 8, 2025

Motivation

Description

  • Document reason behind why we skip some tests
  • Improve all.rs code overall
  • Fix tests that we were skipping and we shouldn't have been

We fix most of the remaining tests in follow-up PRs to this. The tests that have been fixed here had short fixes.

@JereSalo JereSalo self-assigned this Oct 8, 2025
@github-actions github-actions bot added the L1 Ethereum client label Oct 8, 2025
Copy link

github-actions bot commented Oct 8, 2025

Lines of code report

Total lines added: 3
Total lines removed: 0
Total lines changed: 3

Detailed view
+-----------------------------------+-------+------+
| File                              | Lines | Diff |
+-----------------------------------+-------+------+
| ethrex/crates/common/trie/trie.rs | 905   | +3   |
+-----------------------------------+-------+------+

Copy link

github-actions bot commented Oct 8, 2025

Benchmark for c71f66f

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 37.0±4.13ms 37.4±4.47ms +1.08%
Trie/cita-trie insert 1k 3.8±0.05ms 3.8±0.07ms 0.00%
Trie/ethrex-trie insert 10k 65.6±3.97ms 65.8±2.85ms +0.30%
Trie/ethrex-trie insert 1k 6.7±0.07ms 6.7±0.12ms 0.00%

Copy link

github-actions bot commented Oct 8, 2025

Benchmark for f1162b1

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 35.7±1.32ms 35.8±1.02ms +0.28%
Trie/cita-trie insert 1k 3.5±0.02ms 3.7±0.20ms +5.71%
Trie/ethrex-trie insert 10k 55.4±0.49ms 55.0±0.99ms -0.72%
Trie/ethrex-trie insert 1k 6.1±0.06ms 6.2±0.09ms +1.64%

Copy link

github-actions bot commented Oct 8, 2025

Benchmark for 661f8a0

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 35.5±0.98ms 35.0±0.87ms -1.41%
Trie/cita-trie insert 1k 3.5±0.02ms 3.6±0.14ms +2.86%
Trie/ethrex-trie insert 10k 55.9±0.76ms 55.2±0.62ms -1.25%
Trie/ethrex-trie insert 1k 6.2±0.06ms 6.2±0.10ms 0.00%

Copy link

github-actions bot commented Oct 9, 2025

Benchmark for 2ab5f08

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 35.2±2.46ms 33.1±2.27ms -5.97%
Trie/cita-trie insert 1k 3.6±0.02ms 3.7±0.18ms +2.78%
Trie/ethrex-trie insert 10k 55.2±0.86ms 56.1±1.11ms +1.63%
Trie/ethrex-trie insert 1k 6.2±0.09ms 6.2±0.16ms 0.00%

Base automatically changed from support_blockchain_tests to main October 9, 2025 14:09
@JereSalo JereSalo changed the title test(l1): fix tests that we should fix and document why we skip some tests test(l1): fix and document tests Oct 9, 2025
Copy link

github-actions bot commented Oct 9, 2025

Benchmark for 146cf2f

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 38.5±3.59ms 36.0±0.77ms -6.49%
Trie/cita-trie insert 1k 3.6±0.02ms 3.6±0.05ms 0.00%
Trie/ethrex-trie insert 10k 54.9±0.72ms 55.8±2.09ms +1.64%
Trie/ethrex-trie insert 1k 6.1±0.01ms 6.1±0.04ms 0.00%

Copy link

github-actions bot commented Oct 9, 2025

Benchmark for 5b4f67e

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 34.7±0.25ms 37.8±2.83ms +8.93%
Trie/cita-trie insert 1k 3.6±0.02ms 3.6±0.26ms 0.00%
Trie/ethrex-trie insert 10k 55.0±0.44ms 55.2±1.62ms +0.36%
Trie/ethrex-trie insert 1k 6.1±0.07ms 6.1±0.14ms 0.00%

Copy link

github-actions bot commented Oct 9, 2025

Benchmark for 777cf13

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 37.3±0.85ms 37.2±1.05ms -0.27%
Trie/cita-trie insert 1k 3.8±0.05ms 3.8±0.03ms 0.00%
Trie/ethrex-trie insert 10k 58.9±0.92ms 58.4±0.80ms -0.85%
Trie/ethrex-trie insert 1k 6.5±0.10ms 6.7±0.31ms +3.08%

@JereSalo JereSalo marked this pull request as ready for review October 10, 2025 11:55
@JereSalo JereSalo requested a review from a team as a code owner October 10, 2025 11:55
@Copilot Copilot AI review requested due to automatic review settings October 10, 2025 11:55
@ethrex-project-sync ethrex-project-sync bot moved this to In Review in ethrex_l1 Oct 10, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves test organization and fixes by documenting test skip reasons and addressing issues with previously skipped tests. The changes focus on better categorizing why certain tests are skipped and fixing some tests that could be easily resolved.

Key changes:

  • Enhanced documentation of test skip reasons with detailed comments explaining why specific tests are skipped
  • Consolidated and reorganized test skip logic across different test modules
  • Fixed a trie implementation issue for empty trie handling

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tooling/ef_tests/state_v2/src/modules/parser.rs Reorganized IGNORED_TESTS array with detailed documentation for each skip reason
tooling/ef_tests/state/parser.rs Changed IGNORED_TESTS from fixed array to slice reference
tooling/ef_tests/blockchain/tests/all.rs Major refactoring of test skip logic with better organization and backend selection
crates/common/trie/trie.rs Added handling for empty trie root hash case

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

Benchmark for 65a142c

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 34.8±1.06ms 34.7±0.25ms -0.29%
Trie/cita-trie insert 1k 3.6±0.02ms 3.6±0.18ms 0.00%
Trie/ethrex-trie insert 10k 55.1±0.86ms 54.9±1.25ms -0.36%
Trie/ethrex-trie insert 1k 6.1±0.01ms 6.1±0.07ms 0.00%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

1 participant