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

fix the tests #21

Closed
wants to merge 3 commits into from
Closed

fix the tests #21

wants to merge 3 commits into from

Conversation

calvinmetcalf
Copy link
Contributor

No description provided.

@kesla
Copy link

kesla commented Sep 11, 2014

Yeah, we have an iterator test in abstract-leveldown these days that fails here since memdown don't have support for snapshots. Take a look at Level/abstract-leveldown#38 for more info.

@kesla
Copy link

kesla commented Sep 11, 2014

Hm... interesting that there are some more tests that fails. Test that I have a feeling that I've written...

@calvinmetcalf
Copy link
Contributor Author

yes there are some issues with buffers it would look like, I put in a quick snapshot fix and there are 6 buffer related tests failing.

I would guess that #20 is related to this.

@calvinmetcalf calvinmetcalf changed the title no real change to see if the tests are just failing by themselves fix the tests Sep 12, 2014
@calvinmetcalf
Copy link
Contributor Author

@kesla I grabed the iterator code from your medeadown plugin and got this working, though that might be something thats better to go into levelup then in each of the adapters

@calvinmetcalf
Copy link
Contributor Author

and by go into levelup I meant abstract leveldown

@kesla
Copy link

kesla commented Sep 12, 2014

@calvinmetcalf 👍 sounds like a good suggestion!

@calvinmetcalf
Copy link
Contributor Author

that being said this is strickly speaking a breaking change as we will now be coercing to buffers where we previously returned strings, @rvagg your opinion?

@nolanlawson
Copy link
Contributor

@calvinmetcalf I think your snapshot changes are going to conflict with my destroy() changes.

And not to say snapshots aren't valuable, but your fix is just copying the entire database into memory every time we iterate. Let's please not fix tests just for the sake of fixing them.

I would much rather just say "memdown does not support snapshots" (ala the new supports() scheme we're discussing in Level/levelup#279) than write a very inefficient iteration implementation.

Think about our own use case: people are using memdown in PouchDB for their unit tests, or to quickly spin up a PouchDB Server and play around with it without writing to disk. Do we really want to copy the entire database every time they call allDocs()?

@calvinmetcalf
Copy link
Contributor Author

closing as this is a bit more complicated

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.

None yet

3 participants