Refactor execution module #5162
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
nrc
force-pushed
the
nrc-cache-mock
branch
2 times, most recently
from
5d8c6b7 to
1554b50
Compare
| assert!( | ||
| first.2.global.artifact_responses.len() < second.2.global.artifact_responses.len(), | ||
| "Second should have all the artifact responses of the first, plus more. first={:?}, second={:?}", | ||
| first.2.global.artifact_responses.len(), | ||
| second.2.global.artifact_responses.len() |
There was a problem hiding this comment.
The assertion on the responses has been lost.
There was a problem hiding this comment.
Yeah, that was deliberate - it did seemed like an implementation detail which shouldn't be visible externally and wouldn't fail unless the assertion which is preserved failed or there is an engine problem
There was a problem hiding this comment.
I added the assertion specifically because I had a bug in the initial draft implementation of the artifact graph where I wasn't correctly moving the responses to exec_state. They don't originate there. It works now. But we have very few tests for behavior across multiple executions where caching comes into play. If you want to delete it, go ahead, but it makes me nervous how few tests exercise this.
Signed-off-by: Nick Cameron <[email protected]>
Signed-off-by: Nick Cameron <[email protected]>
Signed-off-by: Nick Cameron <[email protected]>
Signed-off-by: Nick Cameron <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5162 +/- ##
==========================================
+ Coverage 85.95% 86.02% +0.07%
==========================================
Files 90 92 +2
Lines 32715 32716 +1
==========================================
+ Hits 28121 28145 +24
+ Misses 4594 4571 -23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The goal was to breakup execution/mod.rs which is just too big, encapsulate caching, and generally tidy up starting execution given we have mock execution and caching with some kinda complicated interactions.