-
-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge factory function into testCommon parameter #268
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,15 @@ var db | |
var verifyNotFoundError = require('./util').verifyNotFoundError | ||
var isTypedArray = require('./util').isTypedArray | ||
|
||
module.exports.setUp = function (factory, test, testCommon) { | ||
module.exports.setUp = function (test, testCommon) { | ||
test('setUp common', testCommon.setUp) | ||
test('setUp db', function (t) { | ||
db = factory() | ||
db = testCommon.factory() | ||
db.open(t.end.bind(t)) | ||
}) | ||
} | ||
|
||
module.exports.args = function (test) { | ||
module.exports.args = function (test, testCommon) { | ||
test('test callback-less, 2-arg, batch() throws', function (t) { | ||
t.throws(db.batch.bind(db, 'foo', {}), 'callback-less, 2-arg batch() throws') | ||
t.end() | ||
|
@@ -159,7 +159,7 @@ module.exports.args = function (test) { | |
}) | ||
} | ||
|
||
module.exports.batch = function (test) { | ||
module.exports.batch = function (test, testCommon) { | ||
test('test batch() with empty array', function (t) { | ||
db.batch([], function (err) { | ||
t.error(err) | ||
|
@@ -235,7 +235,7 @@ module.exports.batch = function (test) { | |
}) | ||
}) | ||
} | ||
module.exports.atomic = function (test) { | ||
module.exports.atomic = function (test, testCommon) { | ||
test('test multiple batch()', function (t) { | ||
t.plan(4) | ||
|
||
|
@@ -266,11 +266,11 @@ module.exports.tearDown = function (test, testCommon) { | |
}) | ||
} | ||
|
||
module.exports.all = function (factory, test, testCommon) { | ||
module.exports.all = function (test, testCommon) { | ||
testCommon = testCommon || require('./common') | ||
module.exports.setUp(factory, test, testCommon) | ||
module.exports.args(test) | ||
module.exports.batch(test) | ||
module.exports.atomic(test) | ||
module.exports.setUp(test, testCommon) | ||
module.exports.args(test, testCommon) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to tack on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't standard complain about that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought so as well. |
||
module.exports.batch(test, testCommon) | ||
module.exports.atomic(test, testCommon) | ||
module.exports.tearDown(test, testCommon) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the api is almost identical for all test functions, except for two tests in
test/iterator-range-test.js
which takes an additionaldata
parameter. Which bugs me a bit 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make that internal by only exporting
all
, because this file only hassetUp
,range
andtearDown
- that's one test. We only have to export "subtests" if there are multiple, and if they should be skippable.I don't know if now is the right time, but we can also make an effort to split tests files in such a way that you'd only exclude files, not subtests. Then each file only needs one export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> #270