search

Level / memdown

In-memory abstract-leveldown store for Node.js and browsers.
MIT License
287 stars 37 forks source link

fix the tests #21

Closed calvinmetcalf closed 10 years ago

kesla commented 10 years ago

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 https://github.com/rvagg/abstract-leveldown/issues/38 for more info.

kesla commented 10 years ago

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

calvinmetcalf commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

and by go into levelup I meant abstract leveldown

kesla commented 10 years ago

@calvinmetcalf :+1: sounds like a good suggestion!

calvinmetcalf commented 10 years ago

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 commented 10 years ago

@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 https://github.com/rvagg/node-levelup/issues/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 commented 10 years ago

closing as this is a bit more complicated