Remove location #258

ralphtheninja · 2018-07-10T09:27:01Z

Closes #63

Also see discussion from #256

ralphtheninja · 2018-07-10T09:30:02Z

test/leveldown-test.js

-    )
-    t.end()
-  })
-
  test('test database open no-arg throws', function (t) {


What to do with this file now? It only contains one test which doesn't do anything. I think we have two options here.

We remove it completely

We rewrite it to test other things that might be related to inheritance, such as checking for the complete api and not only .open

Keep it as it is

If we go with 2 or 3, the current test should be renamed because it doesn't have anything to do with throw

I'd personally would go for 1.

+1 to removing

ralphtheninja · 2018-07-10T09:53:00Z

Restarting browser tests a second time.

vweevers · 2018-07-10T09:55:37Z

What about the abstract tests?

abstract-leveldown/test/batch-test.js

Line 8 in 74d7340

db = leveldown(testCommon.location())

etc

ralphtheninja · 2018-07-10T10:01:53Z

~~@vweevers I thought I'd do a second PR for that.~~

ralphtheninja · 2018-07-10T10:02:38Z

~~Can fix in this PR as well if you want.~~

ralphtheninja · 2018-07-10T10:31:12Z

Found this test which assumes location based implementations

abstract-leveldown/test/open-test.js

Lines 84 to 109 in 74d7340

    
           test('test database open errorIfExists:true', function (t) { 
        
             var location = testCommon.location() 
        
             var db = leveldown(location) 
        
             // make a valid database first, then close and dispose 
        
             db.open({}, function (err) { 
        
               t.error(err) 
        
               db.close(function (err) { 
        
                 t.error(err) 
        
                 // open again with 'errorIfExists' 
        
                 db = leveldown(location) 
        
                 var async = false 
        
                 db.open({ createIfMissing: false, errorIfExists: true }, function (err) { 
        
                   t.ok(err, 'error') 
        
                   t.ok(/exists/.test(err.message), 'error is about already existing') 
        
                   t.ok(async, 'callback is asynchronous') 
        
                   t.end() 
        
                 }) 
        
                 async = true 
        
               }) 
        
             }) 
        
           })

Will comment out this for now.

ralphtheninja · 2018-07-10T10:49:28Z

test/open-test.js

-      })
-    })
-  })
+  // TODO this assumes location based backends, move out to respective


This is the only test that assumes the (previous) leveldown parameter to accept a location argument (so it can try and open on the same location). We should either remove this and move to location based implementations, or move it to a location based file (e.g. test/leveldown-test.js).

However, it uses errorIfExists property which afaik is related to leveldown, so maybe we should just move it to LevelDB based implementations.

Hm, why does this test need two db instances? If you open a db, close it, then reopen with errorIfExists, it should error too, right?

(Removing createIfMissing and errorIfExists from abstract-leveldown is probably a discussion for later)

I'm in a rush right now. Will get back to this in a bit.

@vweevers Reverted the commit and brought back the test using a single db instance.

ralphtheninja · 2018-07-10T10:51:38Z

@vweevers I could use some feedback on the TODO comments specifically.

ralphtheninja · 2018-07-10T11:45:49Z

Left to do:

~~some notes in UPGRADING.md on api changes~~ Skipping this to next PR.
address TODO comments

vweevers · 2018-07-10T18:12:39Z

README.md

 function FakeLevelDOWN (location) {
-  AbstractLevelDOWN.call(this, location)
+  this.location = location


Might as well remove this too

vweevers · 2018-07-10T18:17:22Z

test/common.js

 var tempy = require('tempy')

 module.exports = {
+  // TODO remove?


Removing SGTM. Implementations only have to call tempy.directory in their factory.

Yep! Also means we can merge common.js and common-browser.js into one file.

Oh but we should document somewhere that each factory call should return a fresh db

Or add a test for this behavior :)

vweevers · 2018-07-10T18:18:47Z

Would it make sense for factory to be part of testCommon?

ralphtheninja · 2018-07-10T18:28:31Z

Would it make sense for factory to be part of testCommon?

It might make sense actually. The api would be a lot cleaner since (factory, test) and (factory, test, testCommon) could just be (test, testCommon).

Maybe merge those in another PR?

vweevers · 2018-07-10T18:30:12Z

Maybe merge those in another PR?

👍

vweevers · 2018-07-10T18:31:39Z

I love where this is going!

ralphtheninja · 2018-07-10T18:34:58Z

I love where this is going!

Haha same here. Things just seemed to fall in place a lot when location was no longer something to consider.

vweevers · 2018-07-10T18:45:31Z

test/open-test.js

@@ -80,35 +80,6 @@ module.exports.openAdvanced = function (factory, test) {

    async = true
  })
-
-  // TODO this assumes location based backends, move out to respective


I'm not sure about this, see #258 (comment)

ralphtheninja · 2018-07-10T20:10:54Z

I added the tag pre-remove-location to current master. Creating a abstract-leveldown-v6 branch in leveldown to try out the new changes to see if we are running into something unforeseen. Keeping WIP status of this PR for a while.

This test should be implemented in location based implementations, e.g. leveldown, leveldown-hyper, rocksdb etc.

This reverts commit fceb64e.

ralphtheninja · 2018-07-10T22:49:56Z

Rebased onto latest master (without node 9 so faster builds yay!)

ralphtheninja · 2018-07-11T10:45:34Z

test/common.js

@@ -1,9 +1,4 @@
-var collectEntries = require('level-concat-iterator')
-var tempy = require('tempy')
-
 module.exports = {


Maybe we could export a function here instead? E.g.

module.exports = function (opts) { return xtend({ setUp: function (t) { t.end() }, tearDown: function (t) { t.end() } }, opts) }

ralphtheninja · 2018-07-11T11:09:47Z

@vweevers Sorry to be a pain. Good to squash?

vweevers · 2018-07-11T11:32:32Z

Can you give me until tonight (~6 hours from now)?

ralphtheninja · 2018-07-11T12:28:40Z

Can you give me until tonight (~6 hours from now)?

For sure!

vweevers · 2018-07-11T19:45:50Z

test/open-test.js

@@ -82,20 +82,17 @@ module.exports.openAdvanced = function (leveldown, test, testCommon) {
  })

  test('test database open errorIfExists:true', function (t) {
-    var location = testCommon.location()
-    var db = leveldown(location)
+    var db = factory()

    // make a valid database first, then close and dispose


Leftover comment: " and dispose"

Removing the comments altogether.

vweevers · 2018-07-11T19:47:55Z

@ralphtheninja Can you open issues for:

Document that each factory call must return a fresh db
Test that each factory call returns a fresh db (open two dbs, write to first, check that second is empty)
Consider exporting testCommon as function
Update upgrade guide (also for seek)

ralphtheninja commented Jul 10, 2018

View reviewed changes

ralphtheninja requested a review from vweevers July 10, 2018 09:31

ralphtheninja changed the title ~~Remove location~~ Remove location [WIP] Jul 10, 2018

ralphtheninja commented Jul 10, 2018

View reviewed changes

vweevers reviewed Jul 10, 2018

View reviewed changes

ralphtheninja changed the title ~~Remove location [WIP]~~ Remove location Jul 10, 2018

ralphtheninja added 7 commits July 11, 2018 00:44

Remove location

bf3d49f

Update UPGRADING.md with location changes

5a57289

Replace leveldown(testCommon.location()) with factory()

5d1d61a

Remove testCommon parameter and require level-concat-iterator

c0dea1f

Remove TODO comment

4f7a5fa

Remove testCommon parameter and require level-concat-iterator 2

8cda5d8

Remove collectEntries from common{,-browser}.js, no longer needed

378477b

ralphtheninja added 6 commits July 11, 2018 00:44

Remove location from common{,-browser}.js, no longer needed

af73547

common.js and common-browser.js are identical so can be merged

4749d33

Remove location from FakeLevelDOWN example in README

69667d4

Remove location based test

59753b2

This test should be implemented in location based implementations, e.g. leveldown, leveldown-hyper, rocksdb etc.

Revert "Remove location based test"

1a02c9e

This reverts commit fceb64e.

Bring back errorIfExists test but use same db instance

3dc4cd7

ralphtheninja force-pushed the remove-location branch from 49b7b2d to 3dc4cd7 Compare July 10, 2018 22:44

ralphtheninja commented Jul 11, 2018

View reviewed changes

vweevers reviewed Jul 11, 2018

View reviewed changes

vweevers approved these changes Jul 11, 2018

View reviewed changes

Cleanup comments

2ca95d5

This was referenced Jul 11, 2018

Document test suite #259

Closed

Test that each factory call returns a unique db #260

Closed

Consider exporting test/common as a function #261

Closed

Update upgrade guide #262

Closed

ralphtheninja merged commit aa7e0b8 into master Jul 11, 2018

ralphtheninja deleted the remove-location branch July 11, 2018 21:26

vweevers mentioned this pull request Jul 13, 2018

document .location property #113

Closed

Remove location #258

Remove location #258

Conversation

ralphtheninja commented Jul 10, 2018

ralphtheninja Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

ralphtheninja Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralphtheninja commented Jul 10, 2018

vweevers commented Jul 10, 2018

ralphtheninja commented Jul 10, 2018 • edited Loading

ralphtheninja commented Jul 10, 2018 • edited Loading

ralphtheninja commented Jul 10, 2018

ralphtheninja Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralphtheninja commented Jul 10, 2018

ralphtheninja commented Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vweevers commented Jul 10, 2018

ralphtheninja commented Jul 10, 2018

vweevers commented Jul 10, 2018

vweevers commented Jul 10, 2018

ralphtheninja commented Jul 10, 2018

Choose a reason for hiding this comment

ralphtheninja commented Jul 10, 2018

ralphtheninja commented Jul 10, 2018

Choose a reason for hiding this comment

ralphtheninja commented Jul 11, 2018

vweevers commented Jul 11, 2018

ralphtheninja commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vweevers commented Jul 11, 2018

ralphtheninja Jul 10, 2018 •

edited

Loading

ralphtheninja Jul 10, 2018 •

edited

Loading

ralphtheninja commented Jul 10, 2018 •

edited

Loading

ralphtheninja commented Jul 10, 2018 •

edited

Loading

ralphtheninja Jul 10, 2018 •

edited

Loading

ralphtheninja commented Jul 10, 2018 •

edited

Loading