Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non filesystem store #540

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

djerius
Copy link
Contributor

@djerius djerius commented Aug 18, 2017

This PR adds a new non-filesystem based backend (based on Archive::Tar) as an example and test target. The store tests have been modified so that multiple backends may be tested, and no longer assume that the backend is file system based. The Store API has been cleaned up to make it agnostic to how the backend manages the files.

we only care about files anyway, so moving the file check into files()

a) ensures that files() lives up to its name; and

b) makes find_files a bit more generic, so that it eventually doesn't
   have to check the filesytem at all.  it still calls _check_exists.
Users of Store shouldn't have direct access to a file, as it prevents
Store from using non-filesystem based backends.
Not all store backends will be filesystem based, so using Path::Tiny's
realpath method won't work for them.
…ork with it

A new storage backend, Store::Archive::Tar is included as an example
and to allow testing.

In order to accomodate testing of multiple backends the t/store
tests were converted into Moo::Role's.  These are applied to a
My::Test::Store class which is instantiated for each storage backend.
The roles are discovered via Mojo::Loader, so it's simple to add new
ones.  The disadvantage to this approach is that the tests are no
longer easily parallalizeable.

The t/store test suite assumed filesystem based storage, and often
went directly to disk rather than use the Store API.  The suite now
uses the Store API exclusively. There was one addition to the API to
support this, namely Store::read_file_raw(), which unlink read_file()
peforms no decoding of the data from the file.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5da8a79 on djerius:non-filesystem-store into ** on preaction:master**.

@preaction
Copy link
Owner

This is a pretty big commit, so I've not yet gotten all the way through it, but a quick question: Are you intending on using this Store in your site? If so, okay, I'll continue reviewing. But if not, there are easier ways to test that the Store API works no matter how you override the methods inside (so, trying to lock down the API).

@djerius
Copy link
Contributor Author

djerius commented Aug 24, 2017

The actual Store I'm working on uses a version control system as a storage backend. The Archive::Tar store is an exemplar of a non-filesystem store that doesn't require any modules outside of core Perl modules and no external resources, so is easier to incorporate into tests.

I needed something to flush out any assumptions in the Store code about how data was accessed, and the ability to run the Store tests on another implementation was very helpful. For example, it's not obvious from the API that stores can overlap, and that there's a precedence scheme. The tests brought that out.

There are very few changes to the actual Store API; most of the churn is in the tests. The former are worth reviewing. The changes to the tests (other than turning them into roles) were made to isolate them from the storage backend. I think those changes are also valid, although it's harder to see them because of the order of the commits. I can provide a more granular PR which puts them in a clearer context if that would be helpful.

I'm happy to work on a better means of testing the Store API that can be used on alternate Store implementations. What would you suggest?

@djerius
Copy link
Contributor Author

djerius commented Aug 25, 2017

I'm working on better structuring the commits so it's easier to review. Sorry about the state of this one; it escaped late on a Friday afternoon, and it shows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants