chore: Testing Scenario DSL #2011
Conversation
schell
force-pushed
the
chore/pool-testing-dsl
branch
3 times, most recently
from
48e226e to
1b1a6c1
Compare
schell
requested review from
nuttycom and
str4d
and removed request for
nuttycom
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2011 +/- ##
==========================================
- Coverage 67.95% 67.73% -0.23%
==========================================
Files 213 214 +1
Lines 28972 28742 -230
==========================================
- Hits 19688 19468 -220
+ Misses 9284 9274 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
daira
left a comment
daira
left a comment
There was a problem hiding this comment.
utACK with minor suggestions.
nuttycom
left a comment
nuttycom
left a comment
There was a problem hiding this comment.
Blocking on opportunity to review - I will try to get to this today.
| /// - Attempts to construct a request to spend the whole balance to an external address in the | ||
| /// same pool. | ||
| /// - succeds at doing so | ||
| // TODO(schell): as far as I can tell this function is unused in our test suite |
There was a problem hiding this comment.
That's probably a bug. It should be called in the same pattern as the other functions in this module. This is quite error-prone (it's easy to forget to call a test) and I'd like to find a better way of doing it.
There was a problem hiding this comment.
If we don't have any uses of this, we should add one and see whether it passes.
There was a problem hiding this comment.
I added a test for this function on main and it does not pass. I'm guessing that's why it's currently unused?
refactored TestState::generate_next_block in terms of TestState::generate_next_block_multi TestDsl::add_notes_to now takes a generic iterator of notes
nuttycom
left a comment
nuttycom
left a comment
There was a problem hiding this comment.
utACK with naming nits. Also, I would prefer that the DSF -> Dsf capitalization change be reverted because it makes the diff here noisier than it needs to be.
| if note_config.value > zip317::MARGINAL_FEE { | ||
| // Don't include uneconomic (dust) notes in the expected | ||
| // total, as the balance won't include them. | ||
| expected_total = (expected_total + note_config.value).unwrap(); | ||
| } |
There was a problem hiding this comment.
This is a weird place to hardcode this behavior. Maybe the dust threshold should at least be part of the builder, but when we add the ability to consume dust notes in grace inputs then even that would likely break.
Maybe this is okay for now, but it seems like something that will be annoying to track down from test failures if we change how balance calculation is done.
This would be fine if this reflected the documented semantics of balance calculation, but I'm not sure that the balance calculation APIs currently document such a contract.
| /// - Attempts to construct a request to spend the whole balance to an external address in the | ||
| /// same pool. | ||
| /// - succeds at doing so | ||
| // TODO(schell): as far as I can tell this function is unused in our test suite |
There was a problem hiding this comment.
If we don't have any uses of this, we should add one and see whether it passes.
…renamed TestDsl::with_standard_sapling_account to TestDsl::with_sapling_birthday_account
schell
force-pushed
the
chore/pool-testing-dsl
branch
from
5270ee1 to
3645510
Compare
This PR introduces a wrapper over
TestBuilderandTestStatethat provides a scenario-driven API with which to create tests that are easily grokked.So far the test API covers the "add funds" portion of a test scenario.
Existing tests were refactored in terms of this DSL where appropriate.