update abstract-leveldown to v4 #28
Conversation
|
@vweevers Seems we get some EPERM errors on AppVeyor. These are the errors we got on leveldown as well right? I suspect |
test/compact-range-test.js
Outdated
| }) | ||
|
|
||
| test('tearDown', function (t) { | ||
| db.close(testCommon.tearDown.bind(null, 2)) |
There was a problem hiding this comment.
Don't ask. I think my fingers thought I was typing JSON.stringify with an indentation of 2 👼
|
Hmm, there's something fishy here with the destroy-test. After I updated
|
|
Oooooo k. So maybe we do actually have a bug in the code on windows that we haven't noticed until now. Anyone on windows that would like to take a stab? 😀 |
| @@ -57,7 +57,7 @@ makeTest('test destroy() cleans and removes leveldb-only dir', function (db, t, | |||
| db.close(function (err) { | |||
| t.notOk(err, 'no error') | |||
| leveldown.destroy(location, function () { | |||
| t.notOk(fs.existsSync(), 'directory completely removed') | |||
| t.notOk(fs.existsSync(location), 'directory completely removed') | |||
There was a problem hiding this comment.
I wonder if it's a timing issue to use fs.existsSync() on windows or if it's actually a bug. Clearly, not using location is incorrect.
There was a problem hiding this comment.
Though I almost never use fs.exist(Sync), I'm not aware of any timing issues.
It could be the same issue as in leveldown, that some test isn't closing its db, but in leveldown, we saw EBUSY rather than EPERM errors.
No promises as to when, but I'll try to take a look at this.
There was a problem hiding this comment.
Oops, should have read the code, it actually does close the db. Hmm that's great
There was a problem hiding this comment.
Able to reproduce. There are no leftover files, but it seems the directory itself (location) isn't removed.
There was a problem hiding this comment.
So maybe the test should actually check for an empty folder, rather than the folder itself being removed?
There was a problem hiding this comment.
It's worse.. leveldown.destroy actually creates the directory.
There was a problem hiding this comment.
Repro:
test('destroy does not create a directory', function (t) {
t.plan(3)
const location = path.resolve('destroy-test')
rimraf.sync(location)
t.is(fs.existsSync(location), false, 'exists before')
leveldown.destroy(location, function (err) {
t.ifError(err, 'no error')
t.is(fs.existsSync(location), false, 'exists after') // Fails
})
})There was a problem hiding this comment.
The test could check for
- removal of the location folder
- if the folder is not removed, then make sure it's empty
For now, we could do:
fs.readdir(location, (err, files) => {
if (err && err.code === 'ENOENT') // Passed
if (files.length === 0) // Passed
})And open an issue to fix the destroy bug later.
There was a problem hiding this comment.
It's worse.. leveldown.destroy actually creates the directory.
O.o
|
@vweevers So we can merge this for now and take care of the other bug separately. |
|
@ralphtheninja sure |
No description provided.