Closed Saruniks closed 4 months ago
Hi @Xuanwo,
Is it OK to you?
Could you please add a test to verify the field is present? Thanks
Attention: Patch coverage is 65.78947%
with 91 lines
in your changes missing coverage. Please review.
Project coverage is 40.88%. Comparing base (
0cc0c62
) to head (1234cef
). Report is 64 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
src/server.rs | 45.31% | 18 Missing and 52 partials :warning: |
tests/helpers/mod.rs | 78.26% | 10 Missing and 5 partials :warning: |
tests/cache_hit_rate.rs | 91.30% | 6 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi @Saruniks
Is it ready to be merged?
Hello @AJIOB
Ready from my side. Unless perhaps someone has comments on the code and tests.
Hi @sylvestre & @Xuanwo Can we merge it?
i was hoping to have also integration tests like in https://github.com/mozilla/sccache/blob/main/.github/workflows/integration-tests.yml and a verification that the json files contain the fields too
and sorry for the latency
Ok. I'll add the integration test
many thanks :)
Hello,
a) I added integration tests. Not to the GitHub Actions workflow like mentioned above, but "Rust" ones (see tests/cache_hit_rate.rs).
I could add extra jobs to the GitHub Actions workflow, no problems. But I thought that what I developed could be sufficient as no integration with external tool needs to be tested. Let me know if you think otherwise.
b) Regarding the json stats: Apologies for not mentioning it before, but I didn't develop cache hit rate display for json stats.
The problem is that json stats is just serialized structs. When "StatsFormat::Text" stats are achieved by using "complex" print method.
match fmt {
StatsFormat::Text => stats.print(advanced),
StatsFormat::Json => serde_json::to_writer(&mut io::stdout(), &stats)?,
}
I am still not sure what would be the best way to implement cache hit rate for json stats. Maybe custom serialize implementation would work (and not derive trait like currently), but then it could be considered magic-like. But then calculating cache hit rate on every stats change could be considered too wasteful (or considered as calculated value anti-pattern).
So not sure how to proceed with json stats.
don't bother for the json output thanks!
I guess you saw that some tests are failing
I fixed the issue with failing tests. Checks should pass now.
Closes #2161
Inspiration from #614
Feedback is welcome.
No compile requests:
Zero cache hits:
Some cache hits:
sccache --show-adv-stats
: