It's been a while since this repo saw any love, and many dep versions were out of date. I went down this rabbit hole after noticing that the new fsspec broke a highly used test fixture that unfortunately depended on private fsspec APIs (MockFileSystem._strip_protocol)` which had changed. Note we use the newer fsspec in practice in fv3net so this issue is isolated to the test suite.
I removed this mock so that we don't need to maintain it any longer and retargetd at the MemoryFileSystem class which it inherited from.
I also had to change the cache-directory to support URLS like file:///some/path, which the MemoryFileSystem seems to make now. This bug would have occured even if I had repaired the mock gcsfs object.
Debrief: overall, I'm worried about technical debt in this test suite. It took me 6+ hours to do this PR, all to upgrade to a version of fsspec that we know works well with fv3config from extensive production testing. It definitely demonstrates the downsides of using mocks of complex objects like fsspec, depending on private external apis in those mocks, and then widely reusing those mock. If it happens again, I think we may want to be pretty aggressive with deleting/consolidating tests.
I think it's mostly historical baggage at this point since fv3config actually has a rather clean design where 1) configs are compiled into a list of assets and 2) then assets are written to disk. The tests could target (1) and (2) separately so we could test the config handling separately from the I/O/paths/etc.
It's been a while since this repo saw any love, and many dep versions were out of date. I went down this rabbit hole after noticing that the new fsspec broke a highly used test fixture that unfortunately depended on private fsspec APIs (MockFileSystem._strip_protocol)` which had changed. Note we use the newer fsspec in practice in fv3net so this issue is isolated to the test suite.
I removed this mock so that we don't need to maintain it any longer and retargetd at the
MemoryFileSystemclass which it inherited from.I also had to change the cache-directory to support URLS like
file:///some/path, which theMemoryFileSystemseems to make now. This bug would have occured even if I had repaired the mock gcsfs object.Debrief: overall, I'm worried about technical debt in this test suite. It took me 6+ hours to do this PR, all to upgrade to a version of fsspec that we know works well with fv3config from extensive production testing. It definitely demonstrates the downsides of using mocks of complex objects like fsspec, depending on private external apis in those mocks, and then widely reusing those mock. If it happens again, I think we may want to be pretty aggressive with deleting/consolidating tests.
I think it's mostly historical baggage at this point since fv3config actually has a rather clean design where 1) configs are compiled into a list of assets and 2) then assets are written to disk. The tests could target (1) and (2) separately so we could test the config handling separately from the I/O/paths/etc.