Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ast): derive PartialEq and Eq for testing #259

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

39555
Copy link
Contributor

@39555 39555 commented Nov 5, 2024

#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

- only applies when `cfg_attr(test)`
Copy link

github-actions bot commented Nov 5, 2024

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
Copy link
Owner

reubeno commented Nov 5, 2024

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
Copy link
Contributor Author

39555 commented Nov 5, 2024

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
Copy link
Owner

reubeno commented Nov 5, 2024

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.

@reubeno reubeno merged commit a37e470 into reubeno:main Nov 5, 2024
17 checks passed
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