Skip to content
Open
9 changes: 8 additions & 1 deletion crates/forge-runner/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::coverage_api::run_coverage;
use crate::forge_config::{ExecutionDataToSave, ForgeConfig};
use crate::package_tests::TestCase;
use crate::running::{run_fuzz_test, run_test};
use crate::test_case_summary::TestCaseSummary;
use anyhow::Result;
Expand Down Expand Up @@ -56,7 +57,13 @@ const BUILTINS: [&str; 11] = [
];

pub trait TestCaseFilter {
fn should_be_run(&self, test_case: &TestCaseWithResolvedConfig) -> bool;
fn should_be_run<T>(&self, test_case: &TestCase<T>) -> bool
where
T: TestCaseIsIgnored;
}

pub trait TestCaseIsIgnored {
fn is_ignored(&self) -> bool;
}

pub fn maybe_save_trace_and_profile(
Expand Down
11 changes: 10 additions & 1 deletion crates/forge-runner/src/package_tests/with_config.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use super::{TestCase, TestTarget};
use crate::expected_result::{ExpectedPanicValue, ExpectedTestResult};
use crate::{
TestCaseIsIgnored,
expected_result::{ExpectedPanicValue, ExpectedTestResult},
};
use cheatnet::runtime_extensions::forge_config_extension::config::{
Expected, RawAvailableResourceBoundsConfig, RawForgeConfig, RawForkConfig, RawFuzzerConfig,
RawShouldPanicConfig,
Expand All @@ -22,6 +25,12 @@ pub struct TestCaseConfig {
pub disable_predeployed_contracts: bool,
}

impl TestCaseIsIgnored for TestCaseConfig {
fn is_ignored(&self) -> bool {
self.ignored
}
}

impl From<RawForgeConfig> for TestCaseConfig {
fn from(value: RawForgeConfig) -> Self {
Self {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{TestCase, TestTarget};
use crate::expected_result::ExpectedTestResult;
use crate::{TestCaseIsIgnored, expected_result::ExpectedTestResult};
use anyhow::Result;
use cairo_vm::types::program::Program;
use cheatnet::runtime_extensions::forge_config_extension::config::{
Expand Down Expand Up @@ -40,3 +40,9 @@ pub struct TestCaseResolvedConfig {
pub fuzzer_config: Option<RawFuzzerConfig>,
pub disable_predeployed_contracts: bool,
}

impl TestCaseIsIgnored for TestCaseResolvedConfig {
fn is_ignored(&self) -> bool {
self.ignored
}
}
5 changes: 4 additions & 1 deletion crates/forge/src/run_tests/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ async fn test_package_with_config_resolved(
fork_targets: &[ForkTarget],
block_number_map: &mut BlockNumberMap,
forge_config: &ForgeConfig,
tests_filter: &TestsFilter,
) -> Result<Vec<TestTargetWithResolvedConfig>> {
let mut test_targets_with_resolved_config = Vec::with_capacity(test_targets.len());

Expand All @@ -146,7 +147,8 @@ async fn test_package_with_config_resolved(
&forge_config.test_runner_config.tracked_resource,
)?;

let test_target = resolve_config(test_target, fork_targets, block_number_map).await?;
let test_target =
resolve_config(test_target, fork_targets, block_number_map, tests_filter).await?;

test_targets_with_resolved_config.push(test_target);
}
Expand Down Expand Up @@ -175,6 +177,7 @@ pub async fn run_for_package(
&fork_targets,
block_number_map,
&forge_config,
&tests_filter,
)
.await?;
let all_tests = sum_test_cases(&test_targets);
Expand Down
42 changes: 25 additions & 17 deletions crates/forge/src/run_tests/resolve_config.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
use super::maat::env_ignore_fork_tests;
use crate::{block_number_map::BlockNumberMap, scarb::config::ForkTarget};
use crate::{
block_number_map::BlockNumberMap, scarb::config::ForkTarget, test_filter::TestsFilter,
};
use anyhow::{Result, anyhow};
use cheatnet::runtime_extensions::forge_config_extension::config::{
BlockId, InlineForkConfig, OverriddenForkConfig, RawForkConfig,
};
use conversions::byte_array::ByteArray;
use forge_runner::package_tests::{
with_config::TestTargetWithConfig,
with_config_resolved::{
ResolvedForkConfig, TestCaseResolvedConfig, TestCaseWithResolvedConfig,
TestTargetWithResolvedConfig,
use forge_runner::{
TestCaseFilter,
package_tests::{
with_config::TestTargetWithConfig,
with_config_resolved::{
ResolvedForkConfig, TestCaseResolvedConfig, TestCaseWithResolvedConfig,
TestTargetWithResolvedConfig,
},
},
};
use starknet_api::block::BlockNumber;
Expand All @@ -19,28 +24,30 @@ pub async fn resolve_config(
test_target: TestTargetWithConfig,
fork_targets: &[ForkTarget],
block_number_map: &mut BlockNumberMap,
tests_filter: &TestsFilter,
) -> Result<TestTargetWithResolvedConfig> {
let mut test_cases = Vec::with_capacity(test_target.test_cases.len());
let env_ignore_fork_tests = env_ignore_fork_tests();

for case in test_target.test_cases {
let ignored =
case.config.ignored || (env_ignore_fork_tests && case.config.fork_config.is_some());
Copy link
Member

Choose a reason for hiding this comment

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

This could be inlined again

test_cases.push(TestCaseWithResolvedConfig {
name: case.name,
test_details: case.test_details,
config: TestCaseResolvedConfig {
available_gas: case.config.available_gas,
ignored: case.config.ignored
|| (env_ignore_fork_tests && case.config.fork_config.is_some()),
ignored,
fork_config: if ignored && tests_filter.should_be_run(&case) {
None
} else {
resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
.await?
},
Copy link
Member

Choose a reason for hiding this comment

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

I think the condition should be flipped. Also, just checking for if the tests should be run should suffice, we don't need to also check if it was ignored.

Suggested change
fork_config: if ignored && tests_filter.should_be_run(&case) {
None
} else {
resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
.await?
},
fork_config: tests_filter.should_be_run(&case) {
resolve_fork_config(case.config.fork_config, block_number_map, fork_targets)
.await?
} else {
None
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flipped, thanks for mentioning, this sounds more intuitive actually.

expected_result: case.config.expected_result,
fork_config: resolve_fork_config(
case.config.fork_config,
block_number_map,
fork_targets,
)
.await?,
fuzzer_config: case.config.fuzzer_config,
disable_predeployed_contracts: case.config.disable_predeployed_contracts,
},
name: case.name,
test_details: case.test_details,
});
}

Expand Down Expand Up @@ -194,7 +201,8 @@ mod tests {
url: Url::parse("https://not_taken.com").expect("Should be valid url"),
block_id: BlockId::BlockNumber(120),
}],
&mut BlockNumberMap::default()
&mut BlockNumberMap::default(),
&TestsFilter::default(),
)
.await
.is_err()
Expand Down
24 changes: 20 additions & 4 deletions crates/forge/src/test_filter.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use crate::shared_cache::FailedTestsCache;
use anyhow::Result;
use forge_runner::TestCaseFilter;
use forge_runner::package_tests::TestCase;
use forge_runner::package_tests::with_config_resolved::TestCaseWithResolvedConfig;
use forge_runner::{TestCaseFilter, TestCaseIsIgnored};

#[derive(Debug, PartialEq)]
// Specifies what tests should be included
Expand All @@ -26,7 +27,7 @@ pub(crate) enum NameFilter {
}

#[derive(Debug, PartialEq)]
pub(crate) enum IgnoredFilter {
pub enum IgnoredFilter {
NotIgnored,
Ignored,
All,
Expand Down Expand Up @@ -117,9 +118,24 @@ impl TestsFilter {
}
}

impl Default for TestsFilter {
fn default() -> Self {
Self {
name_filter: NameFilter::All,
ignored_filter: IgnoredFilter::All,
last_failed_filter: false,
skip_filter: Vec::new(),
failed_tests_cache: FailedTestsCache::default(),
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If the default is just for tests, let's implement it behind #[cfg(tests)]. I don't think it's a good idea to have a default for this kind of structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put it behind #[cfg(test)]

Copy link
Member

Choose a reason for hiding this comment

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

I think it will be easier to just create this in tests directly, as I wrote in the comment above.


impl TestCaseFilter for TestsFilter {
fn should_be_run(&self, test_case: &TestCaseWithResolvedConfig) -> bool {
let ignored = test_case.config.ignored;
fn should_be_run<T>(&self, test_case: &TestCase<T>) -> bool
where
T: TestCaseIsIgnored,
{
let ignored = test_case.config.is_ignored();

match self.ignored_filter {
IgnoredFilter::All => true,
Expand Down
Loading