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

[testing] Add section: "testing is documentation" #1726

Open
wants to merge 1 commit into
base: master
from

Conversation

@drpicox
Copy link

drpicox commented Feb 8, 2018

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.

But: program testing can be a very effective way to show the presence of bugs, but is hopelessly inadequate for showing their absence. — Edsger W. Dijkstra 1972

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:

  1. We want to change some code, but we cannot guarantee that we will break some functionality because we need also to refactor the test, or there is not test,
  2. We want to change or add some code, but we cannot guarantee to break any functionality because we do not know which are all supported functionalities of our system,

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:

describe('addListener', () => {
  it('should add a callback to the queue', () => {
    dispatcher.addListener(cb);
    expect(dispatcher.queue).toContain(cb);
  });
});

describe('deliver', () => {
  it('should invoke queue callbacks with the received argument', () => {
    dispatcher.queue.push(cb);
    dispatcher.deliver('message');
    expect(cb).toHaveBeenCalledWith('message');
  });
});

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('should deliver messages to listeners', () => {
  dispatcher.addListener(cb);
  dispatcher.deliver('message');

  expect(cb).toHaveBeenCalledWith('message')
});

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.

README.md Outdated
@@ -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.

Copy link
@ljharb

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.

Copy link
@drpicox
README.md Outdated
- 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.

Copy link
@ljharb

ljharb Feb 9, 2018

Collaborator

can you elaborate on what this means?

This comment has been minimized.

Copy link
@drpicox

drpicox Feb 9, 2018

Author

I rewrite it.

This comment has been minimized.

Copy link
@drpicox

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.

README.md Outdated
- 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.

Copy link
@ljharb

ljharb Feb 9, 2018

Collaborator

Can you elaborate on this one?

This comment has been minimized.

Copy link
@drpicox

drpicox Feb 9, 2018

Author

I rewrite it.

This comment has been minimized.

Copy link
@drpicox

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.

@ljharb ljharb added the editorial label Feb 9, 2018
@drpicox drpicox force-pushed the drpicox:testing--is-documentation branch from cfd03a6 to 5a4ac2c Feb 15, 2018
@drpicox drpicox force-pushed the drpicox:testing--is-documentation branch from 5a4ac2c to b635f08 Feb 15, 2018
@drpicox drpicox changed the title [testing] Add testing is documentation [testing] Add section: "testing is documentation" Feb 15, 2018
- 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.

Copy link
@ljharb

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.

Copy link
@drpicox

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.

Copy link
@ljharb

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.

Copy link
@ljharb

ljharb Feb 18, 2018

Collaborator

i'm afraid i'm still not clear on what this one means :-/

This comment has been minimized.

Copy link
@drpicox

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.

Copy link
@ljharb

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.

Copy link
@ljharb

ljharb Feb 18, 2018

Collaborator

Can you elaborate more on what this means?

- 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.

Copy link
@ljharb

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.

Copy link
@drpicox

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.

Copy link
@ljharb

ljharb Apr 29, 2020

Collaborator

Sounds good to me.

@drpicox
Copy link
Author

drpicox commented Feb 20, 2018

I want to review comments, give me some days to review them.
(although any comment is welcome meanwhile)

@picoluna1969

This comment was marked as spam.

1 similar comment
@picoluna1969

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.