search

Level / abstract-leveldown

An abstract prototype matching the leveldown API.
MIT License
146 stars 53 forks source link

Remove .sync from test/put-test.js #285

Closed ralphtheninja closed 6 years ago

ralphtheninja commented 6 years ago

https://github.com/Level/abstract-leveldown/blob/48484ce12e8e59c0665b50b4bc085809bd662cf4/test/put-test.js#L97-L117

Does this have any implications? Should it be moved to leveldown?

vweevers commented 6 years ago

Should it be moved to leveldown?

I vote yes.

But please wait with touching the tests, I'm currently updating them for #277.

ralphtheninja commented 6 years ago

But please wait with touching the tests, I'm currently updating them for #277.

Ouch, didn't see your message before fixing https://github.com/Level/abstract-leveldown/pull/286

vweevers commented 6 years ago

Ah, it's fine, I'll rebase. I don't want to rush #277, but it also shouldn't block other tasks

vweevers commented 6 years ago

While we're at it, I don't understand what this sync test actually tests. It does two tests. The first is just a put() with a sync option, but the result would be the same if you leave out sync. Might be better to test that the sync option is passed on to _put(). The second test seems to assume that sync means synchronous, but it isn't (in Node.js).

ralphtheninja commented 6 years ago

I don't either. Doesn't really have anything to do with the sync option.