(Chore): Reorganize report tests#1099
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1099 +/- ##
==========================================
+ Coverage 81.90% 82.15% +0.25%
==========================================
Files 69 69
Lines 15039 15039
==========================================
+ Hits 12317 12356 +39
+ Misses 2722 2683 -39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
I realize this is an antipattern in rust-world, but it matches the pattern of the rest of this repo, and I didn't want to undertake crate explosion right now. Having these common utils are necessary for the other "integration" tests in this crate.
There was a problem hiding this comment.
what about this is an anti-pattern? new to rust
There was a problem hiding this comment.
Ideally you don't define re-usable functions for tests within the tests directory. It forces one to mark the functions as dead_code because code (the test functions themselves) within tests files only materializes via compiler options.
There was a problem hiding this comment.
The compiler only executes this function inside of tests, which are excluded from the normal compilation cycle, so it thinks this is dead code. The better pattern afaik is to setup separate, flat crates for test_report, test_report_tests, test_report_test_utils, that way the functions are used either directly where they're defined or exposed as public in the crate (and therefore not unused). That's what I tried to do for the rust integration tests in trunk2. But doing so right now in analytics-cli would require a substantial refactor, so I'm just using the macro here to bypass the warning
There was a problem hiding this comment.
Undertook this split mainly to keep the follow-up PR sane. But also just to make it clear whether we're testing the upload flow or the quarantine flow for rspec
There was a problem hiding this comment.
These tests are ported directly with no meaningful changes, just renames and commonizing a couple small pieces
4 participants
Base for #1100
Some reorganization of domains for the report testing (used by RSpec plugin)