Edp/fix test persistant metadata storage - #8
Conversation
mvahowe
left a comment
mvahowe
left a comment
There was a problem hiding this comment.
@ericpyle Thanks for this. In general, it looks good to me, and it works. It's nice to see my persistant metadata test passing!
We need to think about how we structure the project, bearing in mind that, eventually, most of the current repo will be one module in quite a deep hierarchy, and that we need to support inheritance from subclasses we have not yet thought about.
So, eg, there's a file in build/ called setup_burrito_store.js, whjich references FSBurritoStore. At the very least we should call it setup_fs_burrito_store.js and, if this is for tests, I think it should live in the test dir. (If it's a more general helper library I think it should live with the rest of the code which, at some point, needs splitting into subdirectories.)
I'd like to keep the signatures of BurritoStore subclasses as similar as possible. This is so that, eg, we can move most of the unit tests currently in the test_fs_burrito_store.js file into a library that can be run with the constructor for any particular subclass of burrito store as an argument. If we need to instantiate stores in two operations, or if we need a wrapper function to do that, that's fine, but I'd like that process to be broadly the same for sql or s3 or mongodb. (The details of the config file will change, and we may need extra arguments, which is fine.)
Re your comment about sDir, I may be misunderstanding the issue, but I think we should avoid going async-happy. Assigning a string to a member variable doesn't feel to me like a blocking operation, and indeed I wouldn't be surprised if the overhead of starting up async was greater than that of an assignment operation. Also, bear in mind that, in all the use cases I can think of, there will be several layers of code above the store. So, eg, an HTTP request will probably get wrapped in an async before this module is even called. File operations are a classic place to use async, but I don't think we want to make every function async because we end up with async nested 10 levels deep, which increases overheads and complexity for no benefit over a single, high-level async. In general, I think we want async for the "public" endpoints.
move package.json eslintConfig to test/.eslintrc.json # Conflicts: # code/fs_ingredients_store.js # code/fs_metadata_store.js # express/app.js # fs_burrito_store.js # package.json # test/package.json # test/test_bundle_import.js # test/test_fs_burrito_store.js
2 participants
@mvahowe I've begun adding general support for async - await, so that async await can start to be used for filesystem operations.
This involves some restructuring of mocha tests. Doesn't seem to be too bad to me, but I had to completely disable the rule to not use arrow functions in mocha tests.
mocha/no-mocha-arrowsand get rid of references tothis.One possible implication is that references to
sDirshould be moved out of the contructor and into the factory method for each class, since usage ofsDiris likely to block for too long.