search

automerge / automerge-repo

MIT License
419 stars 43 forks source link

Tests suite for storage adapters #307

Closed bijela-gora closed 3 months ago

bijela-gora commented 4 months ago

This test suite is based on the @acurrieclark comment:

the only reason we know the existing adapters work is because we are using them in production!

To get production data, I launched the React to-do app from the example directory. I also launched the Node.js server and recorded calls to each public method of the NodeFSStorageAdapter instance. That recordings found its place in the test suite. I made buffers smaller for simplicity.

The reason why the storage adapter test suite function has such an interface and is used in this specific way is because I think it should be possible to execute suite hooks. For example, I think it's a good idea to clean up temp files after the execution of each test. The suit hooks are a good place for this, because they will be called even if a test fails. But it seems that for testing the NodeFSStoradgeAdapter, calling the suite hook is not necessary.

In addition to all above I have added comments to explain the decision behind and reasons for a change.

bijela-gora commented 4 months ago

I added some tests based on the issues I noticed while working on the LevelDbStoradgeAdapter in the side branch.

bijela-gora commented 3 months ago

While writing the SQLite storadge adapter. I encountered an issue that forces me to ask a question again. Is this test which I added based on the NodeFSStorageAdapter behavior correct? Is this behavior expected?

bijela-gora commented 3 months ago

@HerbCaudill in this comment you wrote:

i think it would be a good idea to restructure the storage adapter tests that currently live in StorageSubsystem.test.ts so that they work more like the network adapter acceptance tests here

I can say that I did that in this PR. Could I ask you to review it?

bijela-gora commented 3 months ago

Just a kind reminder: @HerbCaudill could you please review this PR?

HerbCaudill commented 3 months ago

While writing the SQLite storadge adapter. I encountered an issue that forces me to ask a question again. Is this test which I added based on the NodeFSStorageAdapter behavior correct? Is this behavior expected?

https://github.com/automerge/automerge-repo/blob/dec8267441a1280b8911e08f8eabe37664fd37d1/packages/automerge-repo/src/helpers/tests/storage-adapter-tests.ts#L145-L158

I'm pretty sure this is the behavior I would expect - @pvh @alexjg @alexcc thoughts?

HerbCaudill commented 3 months ago

Hi, @bijela-gora - thanks for working on this, looks good and does what it needs to do. I'd like to organize things a little differently, can you allow me to push commits to this PR branch?

bijela-gora commented 3 months ago

@HerbCaudill thank you for investing time in review.

I ticked the 'Allow edits by maintainers' checkbox. Should work now.

HerbCaudill commented 3 months ago

Hi, @bijela-gora - I've pushed my changes, let me know what you think.

I've changed the API to match the existing network acceptance tests: runStorageAdapterTests takes a setup function, which returns an adapter and a teardown function. We use this pattern in tests throughout the codebase. I like this better than before/after hooks that modify global variables, because it makes it harder for tests to accidentally interfere with each other via shared context.

I've also changed the keys, payloads etc. to make the tests as easy to read as possible:

I've also added a large payload test with randomly generated data to make sure that everything works with realistic payload sizes as well.

HerbCaudill commented 3 months ago

I also rebased on latest main branch so it can be merged.

Let me know if this all looks good to you, and I'll merge this PR.

bijela-gora commented 3 months ago

@HerbCaudill thank you! Let's merge?