feat(ast): derive `PartialEq` and `Eq` for testing

[39555]

254

Instead of using insta with text representation we can actually use assert_eq! with a direct snapshot of the data structure. I've added one example parser::tests::test_parse_programto show how it looks like.

The #[derive] only applies when cfg_attr(test) is enabled.

benefits over insta

• simple
• no external tools needed
• rust-analyzer can rename fields
• syntax highlighting/formatting

[github-actions[bot]]

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
expand_one_string	3.46 μs	3.46 μs	0.00 μs	⚪ Unchanged
instantiate_shell	60.45 μs	60.27 μs	-0.18 μs	⚪ Unchanged
instantiate_shell_with_init_scripts	30514.88 μs	31284.88 μs	770.00 μs	⚪ Unchanged
parse_bash_completion	2824.55 μs	2879.87 μs	55.32 μs	🟠 +1.96%
parse_sample_script	4.33 μs	4.48 μs	0.14 μs	⚪ Unchanged
run_echo_builtin_command	91.46 μs	90.57 μs	-0.89 μs	⚪ Unchanged
run_one_builtin_command	109.57 μs	108.56 μs	-1.01 μs	⚪ Unchanged
run_one_external_command	1911.29 μs	1963.54 μs	52.25 μs	🟠 +2.73%
run_one_external_command_directly	1007.32 μs	1005.29 μs	-2.03 μs	⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-core/src/jobs.rs	🔴 42.42%	🔴 37.23%	🔴 -5.19%
brush-parser/src/parser.rs	🟢 99.07%	🟢 99.14%	🟢 0.07%
Overall Coverage	🟢 77.74%	🟢 77.76%	🟢 0.02%

Minimum allowed coverage is 70%, this run produced 77.76%

[reubeno]

I like the idea of deriving Eq and PartialEq for test configurations! Coupled with use of pretty_assertions, which I started using in brush-parser, we'll get decent easy-to-read failure output.

As for the actual tests, I do like the idea of insta or similar to make it easier to generate/maintain the snapshots. What do you think?

I'm supportive of moving forward with this change--if you'd like to see it get in--and incrementally later more deeply evaluate adopting insta or similar.

[39555]

Lets go with Eq/PartialEq for now. If we find we need insta, we can give it a try. I copied the snapshot from the Debug output, so generating is not a big deal.

By the way there is a strange failed test on debian, I don't understand why..

[reubeno]

I've queued the failing Debian check for a re-run; we'll see if it succeeds. If it does, then this may mean the test in question is flaky.

I copied the snapshot from the Debug output, so generating is not a big deal.

For now that's fine, but I don't think that scales. I can also see value in having the test cases defined in their own easy-to-read (and grep) subdirs. Before we add too many more cases, we should look into insta or similar.