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

Tests suite for storage adapters #307

Merged
merged 28 commits into from
Mar 27, 2024
Merged

Tests suite for storage adapters #307

merged 28 commits into from
Mar 27, 2024

Conversation

bijela-gora
Copy link
Contributor

@bijela-gora bijela-gora commented Mar 8, 2024

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 bijela-gora marked this pull request as ready for review March 8, 2024 23:42
@bijela-gora
Copy link
Contributor Author

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

@bijela-gora
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@HerbCaudill
Copy link
Collaborator

HerbCaudill commented Mar 19, 2024

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?

it("should overwrite data saved with the same key", async () => {
const { adapter, teardown } = await setup()
await adapter.save(["AAAAA", "sync-state", "xxxxx"], PAYLOAD_A)
await adapter.save(["AAAAA", "sync-state", "xxxxx"], PAYLOAD_B)
expect(await adapter.loadRange(["AAAAA", "sync-state"])).toStrictEqual([
{ key: ["AAAAA", "sync-state", "xxxxx"], data: PAYLOAD_B },
])
teardown()
})
})

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

@HerbCaudill
Copy link
Collaborator

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
Copy link
Contributor Author

@HerbCaudill thank you for investing time in review.

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

@HerbCaudill
Copy link
Collaborator

HerbCaudill commented Mar 24, 2024

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 substituted the real-world keys with more mnemonic fake ones like AAAAA and xxxxx
  • I've truncated the payloads and stored them in named constants like PAYLOAD_A, to reduce visual noise and make it easier to see what is going on

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
Copy link
Collaborator

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
Copy link
Contributor Author

bijela-gora commented Mar 26, 2024

@HerbCaudill thank you! Let's merge?

@HerbCaudill HerbCaudill merged commit 32fd1dc into automerge:main Mar 27, 2024
2 checks passed
@bijela-gora bijela-gora deleted the tests/for-nodefs-storage branch March 27, 2024 19:26
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.

2 participants