reubeno / brush

bash/POSIX-compatible shell implemented in Rust
MIT License
26 stars 4 forks source link

Use `insta` or `expect_test` for snapshot based testing of the parser #254

Open 39555 opened 2 weeks ago

39555 commented 2 weeks ago

What do you think about using either insta , see examples:

or rust-analyzer's expect-test?

This approach is based on matching a Debug or any text representation, which allows us to reduce assert_matches boilerplate when matching AST nodes. The downside is that if the shape of any node changes, the affected snapshots need to be rebuilt with cargo insta.

reubeno commented 2 weeks ago

I completely agree those parser/tokenizer tests aren't scaling well. I'd previously looked into insta; it looks really promising, just didn't seem like the right fit for our compatibility integration tests. I'm interested in the idea of using it with component-level tests focused on the tokenizer and parser.

(Aside: we'd historically been light on the tokenizer/parser tests, as their results had been less stable--but also less important. As long as we provide the right end-to-end semantics that match bash, we're good to go. With that said, these tokenizer/parser tests can be extremely helpful in pinpointing regressions and unexpected behavior at lower levels. I frequently use VSCode's test running/debugging experiences to step through the code-under-test in some of those tests. Now that things are relatively stable, I support the idea of strengthening these lower-level tests.)

If you (or someone else) is interested, I'd be supportive of seeing a draft PR that demonstrates how we could use insta or similar in one of these cases, with 1 or 2 test cases -- i.e., before jumping ahead to switching everything over to it. Once we can get the right pattern and make sure we've got what we need to run/debug/update tests, then we could move forward to documenting the pattern and adopting it.