Test runner. #1999
Test runner. #1999
Conversation
There was a problem hiding this comment.
What do you think of moving the test runner to the pipeline crate? The we can use it to simplify tests like this
cli/src/test_runner.rs
Outdated
| for (name, (_, val)) in analyzed | ||
| .definitions | ||
| .iter() | ||
| .filter(|(n, _)| n.starts_with("test::test_") || n.contains("::test::test_")) |
There was a problem hiding this comment.
Could you rename the tests in std/math/fp{2,4}.asm to start with test_?
cli/src/test_runner.rs
Outdated
| } | ||
| print!("Running test: {name}..."); | ||
| let function = symbols.lookup(name, &None).unwrap(); | ||
| evaluator::evaluate_function_call::<F>(function, vec![], &mut symbols).unwrap(); |
There was a problem hiding this comment.
I think we could provide a better error message (e.g. mention the name of the test) and continue running the other tests maybe?
Right now, a failing test looks like this:
Running test: std::math::fp2::test::test_add... ok
Running test: std::math::fp2::test::test_mul... ok
thread 'main' panicked at cli/src/test_runner.rs:58:80:
called `Result::unwrap()` on an `Err` value: FailedAssertion("Wrong subtraction result")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Running test: std::math::fp2::test::test_sub...%
(test_sub failed)
There was a problem hiding this comment.
Oh interesting! I thought we implemented the built-in panic by using panic!(), but we actually return an Error. How convenient!
cli/src/main.rs
Outdated
| /// Also run the tests inside the standard library. | ||
| #[arg(long)] | ||
| #[arg(default_value_t = false)] | ||
| include_std_tests: bool, |
There was a problem hiding this comment.
It filters tests based on their namespace. Since std is always included, we need to filter the tests out. If you run powdr test std with the filter active, it will not run any tests.
There was a problem hiding this comment.
Hmm, but does this need to leak to the CLI? Or could we just say we never want to run the std tests, unless we pass std explicitly?
cli/src/main.rs
Outdated
| /// Also run the tests inside the standard library. | ||
| #[arg(long)] | ||
| #[arg(default_value_t = false)] | ||
| include_std_tests: bool, |
There was a problem hiding this comment.
Hmm, but does this need to leak to the CLI? Or could we just say we never want to run the std tests, unless we pass std explicitly?
pipeline/tests/powdr_std.rs
Outdated
| "std::protocols::fingerprint::test::test_fingerprint", | ||
| vec![], | ||
| fn std_tests() { | ||
| let test_count = 9; |
There was a problem hiding this comment.
Is it important to assert that there are 9 tests? In what scenario would it not run all tests? I don't see a good reason and it will be slightly annoying that we have to change this number when adding a new test.
There was a problem hiding this comment.
It's just a safeguard against missing some tests because of the naming scheme or something.
pipeline/tests/powdr_std.rs
Outdated
| assert_eq!( | ||
| test_count, | ||
| run_tests(&std_analyzed::<GoldilocksField>(), true).unwrap() | ||
| ); | ||
| assert_eq!( | ||
| test_count, | ||
| run_tests(&std_analyzed::<Bn254Field>(), true).unwrap() | ||
| ); | ||
| assert_eq!( | ||
| test_count, | ||
| run_tests(&std_analyzed::<BabyBearField>(), true).unwrap(), |
There was a problem hiding this comment.
We also have BabyBear and KoalaBear, actually!
There was a problem hiding this comment.
Also, this approach assumes that all tests should be run on all fields. I think we might have tests in the future that are only supposed to pass on some fields. But we we can also fix that when we get there.
There was a problem hiding this comment.
Sure but then the test function should return early. We might find a more convenient way, though.
| match evaluator::evaluate_function_call::<F>(function, vec![], &mut symbols) { | ||
| Err(e) => { | ||
| let msg = e.to_string(); | ||
| println!("{padding}failed\n {msg}"); |
Simple command line utility that can run tests in a
.asmfile.