Re-enable ignored simtests #8735
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
sadhansood
force-pushed
the
sadhan/fix_simtests
branch
2 times, most recently
from
afbfc7b to
de2a9b5
Compare
| @@ -264,6 +264,9 @@ impl ValidatorProxy for LocalValidatorAggregatorProxy { | |||
| } | |||
|
|
|||
| async fn execute_transaction(&self, tx: Transaction) -> anyhow::Result<ExecutionEffects> { | |||
| if std::env::var("BENCH_MODE").is_ok() { | |||
There was a problem hiding this comment.
So does this require BENCH_MODE to be set outside of simtests? Maybe instead we can run execute_transaction() only in tests and simtests?
There was a problem hiding this comment.
Yes, this change explicitly wants the user to set BENCH_MODE if they want to use execute_bench_transaction() which does not handle epoch changes like the quorum driver. So, the default behavior is favoring correctness (without the env variable set) and allows user to switch to more performant code path if they want to.
There was a problem hiding this comment.
For instance I encountered this locally that stress client started failing after epoch change when i used a small epoch duration
There was a problem hiding this comment.
This is good point. I think this gets into the debate for whether stress client is intended for benchmarking or generating transactions load. If we require users to set BENCH_MODE, I think @asonnino should be aware so he can add the flag when running tests. Also we need to add the flag when running stress client in sui-operations.
| @@ -68,7 +67,6 @@ mod test { | |||
| } | |||
|
|
|||
| #[sim_test(config = "test_config()")] | |||
| #[ignore] | |||
There was a problem hiding this comment.
the ignores that don't have the long comments should actually be left in place. these tests work but are not stable enough to run in CI.
There was a problem hiding this comment.
Is this still unstable after fixing the Narwhal issue?
There was a problem hiding this comment.
ok, i will put those back
sadhansood
force-pushed
the
sadhan/fix_simtests
branch
from
de2a9b5 to
e2da9a6
Compare
sadhansood
force-pushed
the
sadhan/fix_simtests
branch
from
e2da9a6 to
a71a79f
Compare
Description
Describe the changes or additions included in this PR.
Test Plan
How did you test the new or updated feature?
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes