Skip to content

Expand Cache.hs test coverage for cache path, write, and staleness semantics - #22

Merged
iustin merged 3 commits into
mainfrom
copilot/add-tests-for-cache
Sep 6, 2026
Merged

Expand Cache.hs test coverage for cache path, write, and staleness semantics#22
iustin merged 3 commits into
mainfrom
copilot/add-tests-for-cache

Conversation

Copilot AI commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Cache.hs had limited coverage (~40%), with most behavior around cache IO and freshness validation untested. This adds focused tests for the public cache API and staleness checks to exercise the core decision paths.

  • Coverage expansion: cache path + IO primitives

    • Added unit coverage for cachedBasename path construction.
    • Added coverage for writeCacheFile ensuring parent directories are created and bytes are written to the expected target.

  • Coverage expansion: readCacheFile decision matrix

    • Added tests for cache miss behavior (Nothing on absent cache file).
    • Added tests confirming reads still succeed when validation is disabled, even after source updates.
    • Added tests for validation-enabled freshness checks:
      • cache is returned when source/extras are not newer,
      • cache is rejected when source is newer,
      • cache is rejected when any extra dependency is newer.
readCacheFile config (sourcePath config) cacheFn True [extraPath config]
  `shouldReturn` (Nothing :: Maybe BS8.ByteString)

@iustin
Add Cache module read/write and validation tests
fce44ba 
Co-authored-by: iustin <550045+iustin@users.noreply.github.com>
@iustin

iustin commented Sep 6, 2026

Copy link
Copy Markdown
Owner

@copilot see the CI failure.

@iustin
Fix CacheSpec source dir lookup for CI
6a8bd52 
Co-authored-by: iustin <550045+iustin@users.noreply.github.com>

Copilot AI commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@copilot see the CI failure.

I checked the failing run and fixed it in commit 6a8bd52: test/CacheSpec.hs used !! (not in scope with NoImplicitPrelude). I replaced it with headMay/fromMaybe for source-dir lookup. CI should rerun with this fix.

Copilot AI requested a review from iustin September 6, 2026 06:40
@codecov

codecov Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.90566% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.47%. Comparing base (87d5355) to head (0ab2013).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
test/CacheSpec.hs 84.90% 0 Missing and 8 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   55.12%   55.47%   +0.35%     
==========================================
  Files          61       61              
  Lines        6384     6437      +53     
  Branches      406      411       +5     
==========================================
+ Hits         3519     3571      +52     
+ Misses       2462     2455       -7     
- Partials      403      411       +8

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@iustin

iustin commented Sep 6, 2026

Copy link
Copy Markdown
Owner

@copilot - I don't think the threadDelay approach is a good one. Can you switch to explicitly setting mtime on the file in the past, instead?

@iustin
Use explicit mtimes in CacheSpec staleness tests
0ab2013 
Co-authored-by: iustin <550045+iustin@users.noreply.github.com>

Copilot AI commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@copilot - I don't think the threadDelay approach is a good one. Can you switch to explicitly setting mtime on the file in the past, instead?

Updated in commit 0ab2013. I removed the threadDelay usage and switched those staleness tests to explicit mtime control via setFileTimesHiRes.

@iustin
iustin marked this pull request as ready for review September 6, 2026 15:17
Copilot AI lite review requested due to automatic review settings September 6, 2026 15:17
Copilot AI reviewed Sep 6, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The added tests align with the current Cache.hs behavior and improve coverage of key cache IO and freshness decision paths without introducing correctness risks.

Pull request overview

This PR expands the Haskell backend test suite’s coverage around Cache.hs behavior by adding focused unit tests that exercise cache path construction, cache writes, and the staleness/validation decision paths used by readCacheFile.

Changes:

  • Added a unit test for cachedBasename path construction.
  • Added coverage for writeCacheFile ensuring parent directories are created and written bytes match expectations.
  • Added a set of tests covering readCacheFile behavior for cache misses, validation-disabled reads, and validation-enabled freshness checks against source and extra dependencies.
File summaries
File Description
test/CacheSpec.hs Adds new specs for cachedBasename, writeCacheFile, and readCacheFile validation/staleness semantics.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@iustin
iustin merged commit 4c98c53 into main Sep 6, 2026
4 checks passed
@iustin
iustin deleted the copilot/add-tests-for-cache branch September 6, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
+1 more reviewer
@iustin iustin iustin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@iustin iustin

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iustin