DX: allow for progressive cache#7132
Conversation
Wirone
left a comment
There was a problem hiding this comment.
Shouldn't it be tested somehow? But I am not sure it's possible to simulate process interruption... Other workarounds would rather trigger regular cache write.
In terms of configurable interval - for large codebases and/or inefficient file systems it should be possible to opt-out from this feature. Also I believe there's no need to write cache multiple times in CI (if it's used, I don't remember).
| */ | ||
| final class FileCacheManager implements CacheManagerInterface | ||
| { | ||
| public const WRITE_FREQUENCY = 10; |
There was a problem hiding this comment.
Wouldn't it be better to have it dynamic (configurable)? Then it would be easier to test it, but also possible to fine-tune through config?
There was a problem hiding this comment.
for tests - I would suggest to go this part when we would have the good idea for test here.
for fine-tuning - I didn't see the need for that part, actually. I followed YAGNI here. happy to have this configurable if the needs pop up
| @@ -32,6 +32,8 @@ | |||
| */ | |||
There was a problem hiding this comment.
Shouldn't it be tested somehow? But I am not sure it's possible to simulate process interruption...
@Wirone , I failed to do that while remaining sane. happy for contributions here, if anyone has good idea.
There was a problem hiding this comment.
Yeah, I tried to come up with test scenario for that and found it hard too 😉. The only thing that could reflect interruption, would be something like this:
- trigger
fixcommand in a process with timeout, and include some dummy fixer in the ruleset, which would do asleep()for specific file and cause the timeout, but also would leave some mark it was done (creating file?) - trigger another
fixwith the same config, so cache is not invalidated, but this time there wouldn't besleep()and it would go to the end
Test should verify if second run contains S statuses for files processed before timeouted file.
Of course there should be more than 10 files processed in the first run OR write interval should be configurable 😉.
There was a problem hiding this comment.
OR write interval should be configurable 😉.
no changes on my side:
for tests - I would suggest to go this part when we would have the good idea for test here.
Yeah, I tried to come up with test scenario for that and found it hard too 😉.
yeah, we can craft sth like this, but maybe it's overkill...
so, if fixing was interrupted in the middle, we can start from previous moment and not from scratch