[fud2] Rhai Testing #2126
[fud2] Rhai Testing #2126
Conversation
Very minor fix: #2078 added plugins, controlled by a `plugins` key in `fud2.toml`. However, it inadvertently made this option required, i.e., fud2 would crash if this option was not present. Now it's optional! So we just silently proceed with no plugins if none are configured.
Maybe this is wasteful, but it simplifies things a lot.
Clippy was thrown for a loop because of the `to_*` name.
Now the top-level thing is in the builder itself... no need for an extra trait? Not sure this is a good idea.
|
Also, afaict the CI doesn't test features. We should either resolve that, or just remove the feature and enable scripts by default. |
There was a problem hiding this comment.
Nice; I like this idea of just programmatically testing every single op! Hard to go wrong with this, IMO; it's possible that we want a few one-off tests that compose multiple ops in real-world scenarios, but it seems useful to also have this exhaustive approach? Having both kinds of tests can also help with catching the changes in the default path that you mentioned.
I found one small issue with conflating ops with the same input/output states, but that shouldn't be hard to work around!
fud2/tests/tests.rs
Outdated
| #[test] | ||
| fn all_ops() { | ||
| let driver = test_driver(); | ||
| for op in driver.ops.values() { | ||
| let req = fud_core::exec::Request { | ||
| start_file: None, | ||
| start_state: op.input, | ||
| end_file: None, | ||
| end_state: op.output, | ||
| through: vec![], | ||
| workdir: ".".into(), | ||
| }; | ||
| test_emit(&driver, req, &format!("__op_{}", op.name)); | ||
| } | ||
| } |
There was a problem hiding this comment.
No real comment here; just highlighting the critical part of this testing strategy (i.e., loop over all the ops and construct a test for just that op alone, using its input & output state) for other readers.
fud2/tests/tests.rs
Outdated
| @@ -70,23 +84,77 @@ fn req_slug(driver: &Driver, req: &Request) -> String { | |||
| desc | |||
| } | |||
|
|
|||
| fn test_emit(driver: &Driver, req: Request) { | |||
| fn test_emit(driver: &Driver, req: Request, tag: &str) { | |||
| let desc = req_desc(driver, &req); | |||
| let slug = req_slug(driver, &req); | |||
| let ninja = emit_ninja(driver, req); | |||
| insta::with_settings!({ | |||
| description => desc, | |||
There was a problem hiding this comment.
Perhaps the description for these "single-op" tests should also be different? Right now, it sorta assumes the request is based on states, so the description looks like "state1 -> state2". Maybe it would be more informative to just name the state that we are testing.
| build data.dump: dump-to-interp $sim_data | $cider-converter | ||
|
|
||
| build interp_out.json: cider stdin | data.json | ||
| build stdin.json: interp-to-dat interp_out.json | $sim_data interp-dat.py |
There was a problem hiding this comment.
This snapshot illustrates the problem with conflating different ops with the same input/output states: it's trying to use the interp-flat op but ends up using the regular interp op, AFAICT.
fud2/tests/tests.rs
Outdated
| through: vec![], | ||
| workdir: ".".into(), | ||
| }; | ||
| test_emit(&driver, req, &format!("__op_{}", op.name)); |
There was a problem hiding this comment.
I think there may be a minor problem here with reusing test_emit: namely, when there is more than one op with the same pair of input/output states. I guess a simple solution here would be to specify the op we're interested in via through, but we could also consider a different function (similar to test_emit but different) that avoids doing the path-search altogether and just specifies a Plan object directly. The latter might be less error prone and is not very hard to do… something like this:
Plan {
start: "input",
steps: vec![(op, "output")],
workdir: ".",
stdin: false,
stdout: false,
}
|
Made the changes. Should we go ahead and turn on the migration? |
docs/running-calyx/fud2/scripts.md
Outdated
There was a problem hiding this comment.
Awesome! The docs look great.
fud2/tests/tests.rs
Outdated
| /// Emit the string that will be snapshot tested. | ||
| fn emit(self, driver: &Driver) -> String; | ||
|
|
||
| /// Run snapshap test |
| bld.load_plugins().build() | ||
| } | ||
|
|
||
| trait InstaTest: Sized { |
There was a problem hiding this comment.
Very elegant approach here!
I added insta tests that test every defined operation by looping over all the operations in the Driver. Running tests with
migrate_to_featuresfeature enabled, allows you to test that the script versions of the operations do the same thing as the rust version. I caught a few typos in scripts by doing this. @sampsyo let me know what you think of this testing technique. The thing I'm not sure of is if this is adequate to test cases when fud2 goes through multiple operations. This also doesn't test if scripts have changed the default paths between two states.I've also snuck in some improved error messages for when there is an error in some imported submodule. The error now points to the correct file.