Support If-Modified-Since header for feed caching#326
Conversation
|
UPD: No longer relevant |
| }) | ||
| }) | ||
|
|
||
| test('handles 304 not modified response', async () => { |
There was a problem hiding this comment.
I am trying to avoid “unit” test ideology in the project. My dream is to test behavior, not implementation.
What do you think of test scenarios like:
- Mock response and return
Last-Modifiedheader - Run
refresh() - Mock second response and expect to have
If-Modified-Sinceand response with 304 - Run
refresh()again
With this scenario we will check that the whole system works, and we will not need to change test on any implementation changes (like on changing Loader API and Last-Modified storage).
| }) | ||
| }) | ||
|
|
||
| test('handles conditional request when server returns 304', async () => { |
There was a problem hiding this comment.
I like new tests.
The only note: what do you think if we will have the same 1 test here as we have for Atom/JSONFeed (or maybe I am missing something)?
There was a problem hiding this comment.
We added a test case named "handles conditional request when server returns 304" for each of RSS, Atom, and JSON.
For RSS, "sends If-Modified-Since header when refreshedAt is provided" was also added.
Do you mean that it should be removed because it partially duplicates the previous one, i.e., to keep just a single test?
There was a problem hiding this comment.
What is the difference between:
sends If-Modified-Since header when refreshedAt is providedhandles conditional request when server returns 304
in RSS tests?
There was a problem hiding this comment.
The first test verifies the correctness of request formation – whether the correct If-Modified-Since header is sent when refreshedAt is provided.
The second test verifies the correctness of response handling – whether the system properly reacts to a 304 status by returning an empty posts list.
Formally, they serve different purposes. This is an argument for keeping both tests for each feed type.
However, in the current implementation, the second test fully duplicates the first one – it adds a mock 304 response and checks the entire flow. In both tests we call loaders.rss.getPosts, and without refreshedAt both would fail.
But the code might change in the future, so keeping both tests could be useful
There was a problem hiding this comment.
Recent Danny Preussler talk convinced me that unit tests approach is not very helpful. The price of test maintenance is more that it gives in return because we need constantly update tests.
Separated tests for like this is that “unit test” approach. For instance, if we will change loader API, we will need to update test.
The new idea is that tests should test “user side feature” and not implementation (”If it walks like a duck and quacks like a duck, it's a duck”).
So I suggest removing one test and instead have only one test (technically 3, one for Atom, one for RSS, one for JSON Feed) that check both thing.
It will be nice to have in refresh.test.ts another test, which will be like e2e test where we:
- Create feed
- Mock request with 200 and time header and test that it was without header
- Refresh posts
- Mock request with 304 and check that time from step 2 was sent
- Refresh post again
This test will test that app saves feed’s last updated time somewhere and use it again (but do not care about implementation).
Of course, we can do this test in separated PR (or I can generate it with LLM if you don't like tests :D). But I want to share with you this idea of more e2e-ish approach with testing.
There was a problem hiding this comment.
Got it now, thanks – you mentioned that before. Let me add it in the PR
There was a problem hiding this comment.
Added a new test to refresh.test.ts and removed an unit test from rss.test.ts
|
Nice, I like new feature a lot =^_^= Thanks for your awesome work. |
Thank you for this project (and many others), the detailed review, and the valuable feedback |
|
Looks awesome. I will apply it today or tomorrow. If you have Twitter/Mastadon/BlueSky give it, I am going to write about this PR in my accounts. |
|
I cleaned test a lot, it maybe be interesting for you (mostly small tuning) 40e103c |
2 participants
Resolves #324