Skip to content

Conversation

franciszekjob
Copy link
Contributor

@franciszekjob franciszekjob commented Aug 22, 2025

Towards #1431

Stack:

Introduced changes

Introduce parametrized tests by addin #[test_case] attribute.

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@franciszekjob franciszekjob changed the base branch from master to 1431-refactor-with-parsed-values-function August 22, 2025 23:34
@franciszekjob franciszekjob changed the title [WIP] Parametrized tests Parametrized tests Aug 24, 2025
@franciszekjob franciszekjob changed the base branch from 1431-refactor-with-parsed-values-function to 1431-refactor-function-names-generated-by-plugin August 24, 2025 20:48
);
}

// TODO: Use separate process (with oracle) instead of heavy fib calculation.
Copy link
Contributor Author

@franciszekjob franciszekjob Aug 27, 2025

Choose a reason for hiding this comment

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

note: Currently this tests uses same approach as the one in fuzzer - fuzzing_exit_first. However, this is technically non-deterministic and ultimately we should use an external process (oracles) to make one of the tests fail before another.
If you agree with above, I will create and link an issue.

Copy link
Member

Choose a reason for hiding this comment

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

We can create one for that

@franciszekjob franciszekjob marked this pull request as ready for review August 27, 2025 15:53
@franciszekjob franciszekjob requested a review from a team as a code owner August 27, 2025 15:53
@franciszekjob franciszekjob requested review from ddoktorski and MKowalski8 and removed request for a team August 27, 2025 15:53
@franciszekjob franciszekjob mentioned this pull request Aug 28, 2025
5 tasks
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 we should also have an e2e contract test that uses name:.

);
}

// TODO: Use separate process (with oracle) instead of heavy fib calculation.
Copy link
Member

Choose a reason for hiding this comment

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

We can create one for that

Comment on lines +27 to +29
pub fn new(map: &HashMap<SmolStr, Vec<Expr>>) -> Self {
Self(map.clone())
}
Copy link
Member

Choose a reason for hiding this comment

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

IMO it's bit weird to hide a clone like this, I'd prefer an explicit clone at caller side.

use std::sync::LazyLock;

static RE_SANITIZE: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"[^a-zA-Z0-9]+").expect("Failed to create regex"));
Copy link
Member

Choose a reason for hiding this comment

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

Can we even have this kind of invalid characters in the test name itself?

fn sanitize_expr(expr: &Expr, db: &SimpleParserDatabase) -> String {
let expr_text = &expr.as_syntax_node().get_text(db);
let expr_sanitized = RE_SANITIZE
.replace_all(expr_text, "_")
Copy link
Member

Choose a reason for hiding this comment

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

How does the replacement work exactly? If I have multiple consecutive invalid characters, will they be replaced with multiple _ or a single one?


Ok(quote!(
#[implicit_precedence(core::pedersen::Pedersen, core::RangeCheck, core::integer::Bitwise, core::ec::EcOp, core::poseidon::Poseidon, core::SegmentArena, core::circuit::RangeCheck96, core::circuit::AddMod, core::circuit::MulMod, core::gas::GasBuiltin, System)]
#[snforge_internal_test_executable]
Copy link
Member

Choose a reason for hiding this comment

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

What if #[test] managed to execute already before? Won't that duplicate the executables?

Comment on lines +122 to +127
let param_count = func
.declaration(db)
.signature(db)
.parameters(db)
.elements(db)
.len();
Copy link
Member

Choose a reason for hiding this comment

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

This query could be reused, not sure how expensive it is though.

Ok(())
}

fn get_filtered_func_attributes(
Copy link
Member

Choose a reason for hiding this comment

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

If it's filtering out just test attr in case there is no fuzzer then maybe it can be named something more explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Can we also add some multiple attributes tests cases with at least fuzzer, test and test_case present?

And please explicitly test the expansion of this order:

#[test]
#[fuzzer]
#[test_case(...)]

Copy link
Member

Choose a reason for hiding this comment

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

Please also add/expand e2e/plugin_diagnostics.rs tests to check for diagnostics with #[test_case] attribute.

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