Repository navigation
Streams tests guidance #204
Description
Hi Streams folks,
I'd like to get involved and agree that the most pressing thing is probably as many tests as can be mustered so we can start to move forward able to rely on the test suite for correctness of changes and catch regressions etc.
My current plan is work on a branch in a fork of node that simply slings in tests. In the absence of another approach think it best just to start somewhere and reviews can tell me if individual cases are relevant or not.
That being said, having looked at what's there I'd like to agree how to structure these additions. Many of the current tests seem to test lots of things in one file, for example for _writev it does a few different encodings, then cork(), uncork() and it makes me a little nervous. The following seem like important decisions:
- what is the scope of each test?
Should they each do one thing or test all cases of a given feature? - How explicit should tests be? There are any number of assertions one could make - I think the temptation is to assert everything given the separated nature of files but can we assume our coverage is gained over a bunch of files?
- What syntax should these files use? I've been leaning toward using fat arrows in the node repo under the assumption we want to cover the semantics of the current implementation.
Thanks, Alex J Burke.
Activity
mcollina commentedon Apr 26, 2016
Thanks @alexjeffburke! TL;DR get started, and then we review!
A lot of streams tests were written long ago, so they accumulated some cruft. I agree with you that most of the tests are complex and verify a full feature (with a lot of assertions). Also, a lot of things are extremely complex to reason about in the current tests.
IMHO we should have more tests with a narrow scope, any other opinion?
Those were never updated to fat arrows, const and the like. They need updating.
You will probably wait some time until a consensus is reached on this one, so start working on it and send a PR.
calvinmetcalf commentedon Apr 29, 2016
I'd add a single new test first and we can give you some detailed feed back on that one and I'd suggest sticking to single new tests per pull while you're getting the hang of it so that if 3 out of 4 are good but one needs work we can merge 3 without waiting for the 4th to be fixed which may take a while.
alexjeffburke commentedon Apr 30, 2016
Great - I've added a first couple at alexjeffburke/streams-tests. Look forward to the feedback :)
mcollina commentedon Apr 30, 2016
@alexjeffburke send a PR, we will review them there!
alexjeffburke commentedon Apr 30, 2016
nodejs/node#6493