Skip to content
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

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

ralphtheninja
Copy link
Member

Closes #227

require('./test/iterator-test').tearDown(test, testCommon)

require('./test/iterator-range-test').setUp(factory, test, testCommon, [])
require('./test/iterator-range-test').setUp(test, testCommon, [])
Copy link
Member Author

@ralphtheninja ralphtheninja Jul 11, 2018

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 additional data parameter. Which bugs me a bit 😄

Copy link
Member

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 has setUp, range and tearDown - 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> #270

module.exports.batch(test)
module.exports.atomic(test)
module.exports.setUp(test, testCommon)
module.exports.args(test, testCommon)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to tack on testCommon everywhere, even if it's not used in some tests. Just to make it more symmetric.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't standard complain about that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so as well.

@vweevers vweevers merged commit 1c95594 into master Jul 13, 2018
@vweevers vweevers deleted the test-common-and-factory branch July 13, 2018 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants