Make test suite suitable for browsers

[vweevers]

See Level/abstract-leveldown#123 (comment).

[vweevers]

The bigger issue in levelup - for Sauce Labs browser tests, that is - is the use of leveldown in tests.

How about we replace that with memdown @ralphtheninja?

[ralphtheninja]

That's a good idea. It makes levelup less dependent on which system it's being tested on.

[vweevers]

Also means no more setup, cleanup and rimraf :)

[ralphtheninja]

Lets try getting this in before releasing 3.0.0?

[vweevers]

Working on it now

[vweevers]

Hmm, some tests rely on the ability to close and reopen the same database (on disk).

[ralphtheninja]

@vweevers Imo the location isn't needed since it doesn't have anything to do with levelup anymore.

[vweevers]

See my edit, hopefully clarifies the problem.

[vweevers]

For example:

levelup/test/deferred-open-test.js

Lines 134 to 148 in 6614a7c

	'simple ReadStream': function (done) {
	var location = common.nextLocation()
	var db = levelup(encDown(leveldown(location)))
	db.batch(this.sourceData.slice(), function (err) {
	refute(err)
	db.close(function (err) {
	refute(err, 'no error')
	var db = levelup(encDown(leveldown(location)))
	this.closeableDatabases.push(db)
	var rs = db.createReadStream()
	rs.on('data', this.dataSpy)
	rs.on('end', this.endSpy)
	rs.on('close', this.verify.bind(this, rs, done))
	}.bind(this))
	}.bind(this))

[vweevers]

I can rewrite it so it uses one levelup instance. Open it, write the test data, close it, reopen, read, assert data is correct.

[vweevers]

This one is tricky:

levelup/test/read-stream-test.js

Lines 499 to 559 in 6614a7c

	// ok, so here's the deal, this is kind of obscure: when you have 2 databases open and
	// have a readstream coming out from both of them with no references to the dbs left
	// V8 will GC one of them and you'll get an failed assert from leveldb.
	// This ISN'T a problem if you only have one of them open, even if the db gets GCed!
	// Process:
	// * open
	// * batch write data
	// * close
	// * reopen
	// * create ReadStream, keeping no reference to the db
	// * pipe ReadStream through SlowStream just to make sure GC happens
	// - the error should occur here if the bug exists
	// * when both streams finish, verify all 'data' events happened
	'test ReadStream without db ref doesn\'t get GCed': function (done) {
	var dataSpy1 = this.spy()
	var dataSpy2 = this.spy()
	var location1 = common.nextLocation()
	var location2 = common.nextLocation()
	var sourceData = this.sourceData
	var verify = function () {
	// no reference to `db` here, should have been GCed by now if it could be
	assert(dataSpy1.callCount, sourceData.length)
	assert(dataSpy2.callCount, sourceData.length)
	async.parallel([ rimraf.bind(null, location1), rimraf.bind(null, location2) ], done)
	}
	var execute = function (d, callback) {
	// no reference to `db` here, could be GCed
	d.readStream
	.pipe(new SlowStream({ maxWriteInterval: 5 }))
	.on('data', d.spy)
	.on('close', delayed.delayed(callback, 0.05))
	}
	var open = function (reopen, location, callback) {
	levelup(encDown(leveldown(location)), callback)
	}
	var write = function (db, callback) { db.batch(sourceData.slice(), callback) }
	var close = function (db, callback) { db.close(callback) }
	var setup = function (callback) {
	async.map([ location1, location2 ], open.bind(null, false), function (err, dbs) {
	refute(err)
	if (err) return
	async.map(dbs, write, function (err) {
	refute(err)
	if (err) return
	async.forEach(dbs, close, callback)
	})
	})
	}
	var reopen = function () {
	async.map([ location1, location2 ], open.bind(null, true), function (err, dbs) {
	refute(err)
	if (err) return
	async.forEach([
	{ readStream: dbs[0].createReadStream(), spy: dataSpy1 },
	{ readStream: dbs[1].createReadStream(), spy: dataSpy2 }
	], execute, verify)
	})
	}
	setup(delayed.delayed(reopen, 0.05))
	},

[ralphtheninja]

Ouch.

[ralphtheninja]

My brain is kind of mangled now 😄

[vweevers]

It's the last remaining test, I'll try to grok it. IMO we should move it to leveldown because the test comment says:

[otherwise] you'll get an failed assert from leveldb

BTW, the test suite is so much faster now.

[vweevers]

The fix for the described problem (39b44bc) nowadays lives in abstract-leveldown - because a reference to the db is kept in AbstractIterator#db.

But I don't understand the relation between the two db's in the test. I get that V8 will destroy the db if there are no references left, but I don't get why "this ISN'T a problem if you only have one of them open".

By moving the test to leveldown, increasing the test data size, and commenting out this.db = db in AbstractIterator, I've managed to reproduce the LevelDB exception - using 1 db.

[vweevers]

Ah and nowadays we also have a stream-like buffering mechanism with highWatermark in leveldown. I should disable that to ensure data is not read at once (before GC has had a chance to kick in).

[vweevers]

Confirmed. After setting highWaterMark to 0, I no longer need the increased data size.

The test has been useless since highWaterMark was introduced, because the test never triggered GC.