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
src/wasm-lib/tests/executor/cache.rs
Outdated
| 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.
There was a problem hiding this comment.
I'll have a look if there's a way we can test this with a unit test so we don't need to expose that just for testing. Unless you know if there is a proxy for that data which is already exposed?
There was a problem hiding this comment.
The artifact graph is the main output derived from both commands and responses that TS needs. So maybe check that, and responses can stay internal.
Having commands available in TS is really useful for debugging, so I wouldn't make that internal-only.
There was a problem hiding this comment.
Yeah, it seems from the code that the artifact graph should grow iff the responses grow, so I think I can check that
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5162 +/- ##
==========================================
+ Coverage 85.95% 86.02% +0.06%
==========================================
Files 90 92 +2
Lines 32912 32912
==========================================
+ Hits 28291 28314 +23
+ Misses 4621 4598 -23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
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.