Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

convert tests into an abstract test suite module that other levelups can use #273

Closed
max-mapper opened this issue Aug 19, 2014 · 16 comments · Fixed by #677
Closed

convert tests into an abstract test suite module that other levelups can use #273

max-mapper opened this issue Aug 19, 2014 · 16 comments · Fixed by #677
Labels
test Improvements or additions to tests

Comments

@max-mapper
Copy link

It would be really useful for us over at http://github.com/maxogden/dat to be able to e.g. require('abstract-levelup/tests') in the same way that there is an abstract-leveldown test suite.

Publishing a test suite as a module lets multiple modules all ensure compatibility since they use the same test suite. For example, level.js uses abstract-leveldown, and so does memdown and leveldown and others.

I liked @rvagg's idea of a test suite as a module so much that I copied the concept but for file storage: https://github.com/maxogden/abstract-blob-store

However, the levelup tests aren't a module yet. Someone needs to do the heroic task of converting the tests to use the tape API and make the tests require()-able

@kesla
Copy link
Contributor

kesla commented Aug 19, 2014

@maxogden I like to be your hero. I'll take a look at this!

@kesla
Copy link
Contributor

kesla commented Aug 19, 2014

I'm sitting on a plane for 8 hours tomorrow. This will be a great task for those hours.

@ralphtheninja
Copy link
Member

Sweet! We need more programmers on planes :)

@dominictarr
Copy link
Contributor

this would also be of use to level-sublevel

@rvagg
Copy link
Member

rvagg commented Aug 25, 2014

Rather than putting the effort into this I'd like to understand why it's necessary to be building faux-levelups and what is it about levelup now that's preventing you from just reusing it?

I released level-updown yesterday, it's a leveldown backed by levelup, but it's also extensible so you can change its behaviour in creative ways and you end up with an unmodified levelup at the other end and the modifications are all in the leveldown layer (where it's actually easier). I'm using this for level-spaces to give me a namespacing solution where I get to operate on a complete, unmodified levelup but the backend does the translation to and from the namespaces of the original db.

I'd personally like to see more effort put in to creative ways to extend and adapt what we have instead of re-creating new versions that have subtle incompatibilities.

@mafintosh
Copy link
Member

@rvagg Using the approach how would you add new method functionality? For example in dat we have an levelup compatible interface (see level-dat) but we also have a couple of extra methods (like createVersionStream()) exposed on the same instance

@rvagg
Copy link
Member

rvagg commented Aug 27, 2014

@mafintosh I floated this idea for LevelUP a while back but didn't get any traction, I'd be interested to see how far it can get with LevelDOWN. Obviously there's additional functionality you can't achieve with just the LevelDOWN layer so you'd still have to resort to monkey-patching but I also don't see what you can't achieve by just taking LevelUP and monkey-patching or subclassing, why are you making a new one from scratch? If we can identify the problems that are leading you to have to reinvent this wheel then perhaps we can reengineer LevelUP to serve those purposes better. For instance, are we still hiding too much internal logic and data from extensions & subclasses? Should we split up internal functionality into more (pseudo) private methods on the prototype so you can extend it directly and reimplement the methods where you want to adjust the functionality.

@dominictarr
Copy link
Contributor

also, there are elements of levelup you can reuse.
level-sublevel depends on the levelup/lib/codec and levelup/lib/read-stream for example.
there is furthur discussion over here:
#270

@dominictarr
Copy link
Contributor

oh max already linked that discussion here, maybe he has a good point?

@kesla kesla removed their assignment Dec 3, 2014
@max-mapper
Copy link
Author

I still think we need to do this, because just because something has a compatible API it doesn't mean it is implemented correctly.

For example level-spaces does require('levelup') and returns a new instance, so the API is 100% compatible, but it still has bugs like this: https://github.com/rvagg/level-spaces/issues/3

If the test suite of level-spaces could have easily used an e.g. abstract-levelup test suite to check if it's levelup is 100% compatible with the stock levelup then it would have saved me a lot of headache today.

@mcollina
Copy link
Member

👍. After having spent two hours fighting the problem, I think we need such a test suite. Unfortunately I have no bandwidth to help atm.

@juliangruber
Copy link
Member

👍 same as @mcollina

@max-mapper
Copy link
Author

alright i've got a plane ride tomorrow across 'merica, i'll see what i can do

@max-mapper
Copy link
Author

ok I started converting the tests, spent about 4 hours on it: https://github.com/maxogden/abstract-levelup

not done yet, will send a PR when its ready

@rvagg
Copy link
Member

rvagg commented Dec 24, 2014

As I said in dominictarr/level-sublevel#81 I still believe that this desire for this test suite points to a failing of reusability in levelup rather than a concrete need. sublevel should be reusing way more code than it does so it doesn't have to try and conform to the interface. Re-implementing levelup should be considered an anti-pattern and we should instead invest in splitting levelup into smaller bits that can be more easily reused. i.e. #270

@max-mapper
Copy link
Author

@rvagg I definitely see your point, but now we're in a situation where @dominictarr is saying your modules are too complicated, and you are saying his modules are too complicated, so it felt a bit intimidating to get involved with a major overhaul of either module. I also fundamentally disagree with the approach of each of them, for different reasons. I'm liking the approach of https://github.com/mafintosh/subleveldown more, where levelups aren't nested or monkeypatched.

Also, as I said above, even if you require('levelup') in your module, that doesn't guarantee that the levelup instance you expose will pass the levelup test suite. The double-encoding bug in level-spaces is a good example. All I'm trying to do with abstract-levelup is make a test suite that can have a levelup factory passed in and works in browser + node. That way it becomes easy to see if a levelup interface does all the things it should. in any environment.

@vweevers vweevers added test Improvements or additions to tests and removed help wanted Extra attention is needed labels Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test Improvements or additions to tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants