test: Improve test failure messages for isWritable

[philipphofmann]

Slightly improve test failure messages for CacheAdvanceTests when the expect fails when checking isWritable, so it's easier to know why a test failed in CI.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.05%. Comparing base (f729c88) to head (f633117).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #106   +/-   ##
=======================================
  Coverage   99.05%   99.05%           
=======================================
  Files           8        8           
  Lines         529      529           
=======================================
  Hits          524      524           
  Misses          5        5           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dfed]

I'm not convinced this is necessary since the test name is quite clear and #expect does a great job of printing the expectation when it fails, but I don't see harm here.

At a minimum: if we're adding comments, I'd prefer the comments to explain the why rather than the what, since per the above #expect already prints what went wrong on failure. I've left suggestions to that effect.

[philipphofmann]

I'm not convinced this is necessary since the test name is quite clear and #expect does a great job of printing the expectation when it fails, but I don't see harm here.

Yes, it's not really necessary. I was looking at the code and stumbled across it. It's a very minor improvement. It previously failed with

Expectation failed: try sut.isWritable()

Now it fails with

Expectation failed: try sut.isWritable(): Identical cache should be writable

I think that's a slight improvement. Thanks for your input.

[dfed]

CI job failed due to:

The requested device could not be found because no available devices matched the request

I have an issue filed with Github Actions about this. Started this week as far as I can tell. Just re-ran the job and it's good to go. Sorry for the noise there.

I think that's a slight improvement

I agree! Though worth looking at the full failure message context which includes the test name:

Test isWritable_returnsTrueWhenStaticHeaderMetadataMatches() recorded an issue at CacheAdvanceTests.swift:234:3: Expectation failed: try sut.isWritable()

Makes me realize that my use of sut for the variable name of the system under test is not really helping us here. That'd have read better as:

Test isWritable_returnsTrueWhenStaticHeaderMetadataMatches() recorded an issue at CacheAdvanceTests.swift:234:3: Expectation failed: try cacheWithMatchingMetadata.isWritable()

Merging your improvement, but also filing a note to self to improve these test variable names.

[philipphofmann]

We also see these failures on Sentry-Cocoa. I also commented on GH actions actions/runner-images#12948 (comment)

[philipphofmann]

but also filing a note to self to improve these test variable names.

I would say it has low prio 😄. I guess there are more important things to improve.