Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[testing] Add section: "testing is documentation" #1726
Conversation
| @@ -3390,6 +3390,16 @@ Other Style Guides | |||
| - We primarily use [`mocha`](https://www.npmjs.com/package/mocha) at Airbnb. [`tape`](https://www.npmjs.com/package/tape) is also used occasionally for small, separate modules. | |||
| - 100% test coverage is a good goal to strive for, even if it’s not always practical to reach it. | |||
| - Whenever you fix a bug, _write a regression test_. A bug fixed without a regression test is almost certainly going to break again in the future. | |||
|
|
|||
| <a name="testing--is-documentation"></a><a name="30.3"></a> | |||
This comment has been minimized.
This comment has been minimized.
ljharb
Feb 9, 2018
Collaborator
please remove this 30.3 link - new numbered sections should not be added; they're only here for legacy reasons.
This comment has been minimized.
This comment has been minimized.
| - First write a case for each main functionality, and then their exceptions. | ||
| - Refactor tests until they are easy to read and understand. | ||
| - Never use object internal representation from test code, use objects as other code will. | ||
| - Tests should be copy-paste code to make new developments. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
drpicox
Feb 9, 2018
Author
Ops! Nice found, copypaste ≠ copy-pasteable, it seems that I force people to make copy pastes from here, which it is not my objective.
Code inside tests should be copy-pasteable code to make new developments.
| - Tests should explain what your code does. | ||
| - First write a case for each main functionality, and then their exceptions. | ||
| - Refactor tests until they are easy to read and understand. | ||
| - Never use object internal representation from test code, use objects as other code will. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
drpicox
Feb 9, 2018
•
Author
Never use "private" properties to test code.
yes... I elaborate making it shorter O: )
I do not want to break the "rhythm" of the all testing section writing something larger.
I think that it is enough telling to anyone do not use "private" stuff, because they already know what means "private". And if not, and they search for "private" they will find more things later.
cfd03a6
to
5a4ac2c
5a4ac2c
to
b635f08
| - Tests should explain what your code does. | ||
| - First write a case for each main functionality, and then their exceptions. | ||
| - Refactor tests until they are easy to read and understand. | ||
| - Never use _"private"_ properties to test code. |
This comment has been minimized.
This comment has been minimized.
ljharb
Feb 18, 2018
Collaborator
i'm not sure this really belongs; if something is truly private, it's impossible to test directly; and if it's possible to test directly, it's not actually private.
This comment has been minimized.
This comment has been minimized.
drpicox
Feb 20, 2018
•
Author
Uhmmm, I understand your point, I guess that it comes from here:
This convention might lead developers to wrongly think that a change won’t count as breaking, or that tests aren’t needed.
What about changing it for the following statement:
- JavaScript does not have the concept of privacy
in terms of properties or methods,
if you want something to be “private”,
it must not be observably present,
and they won't be used in tests,
see [here](#naming--leading-underscore) for details.
This comment has been minimized.
This comment has been minimized.
ljharb
Feb 22, 2018
Collaborator
I think this is kind of self-evident; and would be more clear with a concrete example of what to avoid.
| - First write a case for each main functionality, and then their exceptions. | ||
| - Refactor tests until they are easy to read and understand. | ||
| - Never use _"private"_ properties to test code. | ||
| - Code inside tests should be copy-pasteable code to make new developments. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
drpicox
Feb 20, 2018
•
Author
The idea is the following: because testing code is documentation, when you start writing new code, you have a very high probability of having that code already implemented in a test. So when someone starts coding and needs to use an existing code, he just have to go the specs, and make copy-paste.
I liked the copy-pasteable expression because it implies that the testing code should be as close as possible to the production code, so each test initialisation, operation and check should be closer to other code initialisation, operation, and querying.
Although this idea, may be it should be clearer something like:
- When someone starts coding
tests are the first place to know how to use existing code,
make them a good starting point
This comment has been minimized.
This comment has been minimized.
ljharb
Feb 22, 2018
Collaborator
I'm still not clear on this. Are you saying that individual tests should be good starting points for adding new tests? I'm not sure this is even broadly true.
| - Never use _"private"_ properties to test code. | ||
| - Code inside tests should be copy-pasteable code to make new developments. | ||
| - If some functionality works but is not tested, do not use it. It may break your code in the future. | ||
| - Test functionalities, not functions. |
This comment has been minimized.
This comment has been minimized.
| - Refactor tests until they are easy to read and understand. | ||
| - Never use _"private"_ properties to test code. | ||
| - Code inside tests should be copy-pasteable code to make new developments. | ||
| - If some functionality works but is not tested, do not use it. It may break your code in the future. |
This comment has been minimized.
This comment has been minimized.
ljharb
Feb 18, 2018
Collaborator
I'd say if it's not tested, then it objectively doesn't work, because you don't have a test proving it does :-)
This comment has been minimized.
This comment has been minimized.
drpicox
Feb 20, 2018
Author
Uhmm, yeah!
What I have found that some times there are some behaviours that emerge from the current code structure, for example:
getCar(carIndex) {
return carList[carIndex];
}It turns out that if you call to getCar(undefined) it returns undefined.
Some people think that to return undefined if argument undefined it is a desired functionality,
and they start to put inputs to getCar that may have an undefined.
Some time later, you may decide to do something else with carIndex that will break if it is undefined.
And then you will break previous code.
And... if they did a lot of mocking, you will not discover it until it is too late (or difficult to trace).
In this scenario coders can say that the function is tested. But if there was no case in which it returns undefined, they never should expected and undefined.
Of course you can add 1.000 cases for each combination, but 1: it is not feasible, 2: when you define an API you define which cases are accepted.
So at the end, specially you should look test code always before using something in some way that was not designed for.
What about:
- If it is not tested, than it objectively does not work,
because you do not have a test proving it does.
Including those cases that are not present.
This comment has been minimized.
This comment has been minimized.
|
I want to review comments, give me some days to review them. |
drpicox commentedFeb 8, 2018
•
edited
It is great to suggest people to write tests, it is really a good idea, but I think that it is not enough.
From my experience working with other coders I have discovered that they consider hard to write tests. Not because it is difficult (you just use the test API like any other library), but because they think that tests are pointless and a waste of time.
I have done a lot of reviews of PR, including those containing tests, and I have discovered very bad testing practices which usually occurs when people are forced to write tests (usually big companies and some test obsessed people). The tests resulting of these practices are almost useless (usually testing code copies production code) and lacks of many of the expected properties of a test.
So tests objective should not be the only one of creating a system bugs free. It is an impossible task from the point of view of that test can effectively achieve, but there are many other objectives which are very valuables.
Main tests objective should be to make legacy code disappear.
I consider legacy code, not just old unsupported code, but any code difficult to change because we may break our system. And as I far as I know it happens in two different scenarios:
Both points can be summarized in good maintainability and good documentation, and we can achieve both with well written tests.
And here comes the good news, there is a simple trick to achieve these two properties: "testing is documentation".
When coders forgot about it, coders usually focus in testing other points. They being obsessed in test function by function all possible cases, all input combinations, all ranges, they try to get the 100% code coverage regardless its usefulness. But all this effort just creates a rigid codes and poorly covered. I think that you tried to make people think better about it when you said: "Be cautious about stubs and mocks", but I it is not enough.
An example of 100% code coverage and almost useless testing code:
These tests achieve 100% of code coverage, even if you suppress expectations in both tests, or replace them by checking anything else, like they both methods return undefined.
If you look carefully these tests tells almost nothing about what to do with the dispatcher, it even exposes some data structures that meant to be internal ( we could implement them with: https://github.com/airbnb/javascript#naming--leading-underscore ). It does not matter how many other test you add (what happens if the queue is empty, if there are many callbacks, if there is no message, ...), you will have hard time trying understand what dispatcher and it will be almost impossible to refactor it without break all tests first (refactor usually implies changing internal representations).
Now check this other test:
It is simple, easy to understand, does not use internal representation, and it has 100% of code coverage.
It should be our objective, and if you look carefully, it is a wonderful documentation. You understand involved steps and see the whole picture. If you want can do copypaste quickly, and build new code from here. And of course, you can refactor the dispatcher code as much as you want without changing a simple line of a test as long as you do not break the dispatcher api compatibility.
Because all exposed here that is the reason why I suggest the addition this section, just to help to create better and more effective testing code. It will mitigate the frustration of many programmers that do not know how to start writing a test and it will give them a direction to follow.
With these few indications I hope that coders will feel good with their own test and give them some deeper meaning that helps them to be proud of their work.
Note: I was tempted to add something about AAA (Arrange Act Assert) or GWT (Give When Then), and force people to split tests that test something more than one thing, but I think that as far as they understand that each test case is an example of usage for the documentation it should not be necessary.