Unpleasant errors while writing unit tests

    The other day I will be doing an internal report in which I will tell our developers about the unpleasant errors that may occur when writing unit tests. The most unpleasant errors from my point of view are when tests pass, but at the same time they do it so incorrectly that it would be better not to pass. And I decided to share examples of such errors with everyone. Surely something else tell me from this area. Examples are written for Node.JS and Mocha, but in general these errors are true for any other ecosystem.

    To make it more interesting, some of them are framed in the form of a problem code and a spoiler, opening which you will see what the problem was. So I recommend that you first look at the code, find an error in it, and then open the spoiler. Solutions to the problems will not be indicated - I suggest you think about it yourself. Just because I'm lazy. The order of the list does not have a deep meaning - it’s just an order in which I remembered all sorts of real problems that brought us to bloody tears. Surely many things will seem obvious to you - but even experienced developers may accidentally write such code.


    So let's go.

    0. Lack of tests


    Oddly enough, many still believe that writing tests slows development speed. Of course, it’s obvious that you have to spend more time writing tests and writing code that can be tested. But debugging and regressing after that will have to spend many times more time ...

    1. No test run


    If you have tests that you do not run, or run from time to time - then this is like the absence of tests. And it's even worse - you have an aging test code and a false sense of security. Tests should at least be run in CI processes when pushing code to a branch. And it is better - locally before we push. Then the developer will not have to return to the build in a few days, which, as it turned out, did not work.

    2. Lack of coverage


    If you still do not know what the coating is in the tests, then it's time to go and read right now. At least Wikipedia . Otherwise there are great chances that your test will test on the strength of 10% of the code that you think it checks. Sooner or later you will definitely step on it. Of course, even a 100% coverage of the code does not guarantee its complete correctness in any way - but this is much better than the lack of coverage, as it will show you much more potential errors. No wonder in the latest versions of Node.JS even appeared built-in tools to count it. In general, the topic of coverage is deep and extremely holivar, but I will not go into it too much - I want to say a little about a lot.

    3 



    const {assert} = require('chai');
constPromise = require('bluebird');
const sinon = require('sinon');
classMightyLibrary{
  static someLongFunction() {
    returnPromise.resolve(1); // just imagine a really complex and long function here
  }
}
asyncfunctiondoItQuickOrFail()
{
  let res;
  try {
    res = await MightyLibrary.someLongFunction().timeout(1000);
  }
  catch (err)
  {
    if (err instanceofPromise.TimeoutError)
    {
      returnfalse;
    }
    throw err;
  }
  return res;
}
describe('using Timeouts', ()=>{
  it('should return false if waited too much', async ()=>{
    // stub function to emulate looong work
    sinon.stub(MightyLibrary, 'someLongFunction').callsFake(()=>Promise.delay(10000).then(()=>true));
    const res = await doItQuickOrFail();
    assert.equal(res, false);
  });
});


    What's wrong here
    Таймауты в юнит тестах.

    Здесь хотели проверить, что установка таймаутов на долгую операцию действительно работает. В целом это и так имеет мало смысла — не стоит проверять стандартные библиотеки — но так же такой код приводит к другой проблеме — увеличению выполнения времени прохождения тестов на секунду. Казалось бы, это не так много… Но помножьте эту секунду на количество аналогичных тестов, на количество разработчиков, на количеств запусков в день… И вы поймёте, что из-за таких таймаутов вы можете терять впустую много часов работы еженедельно, если не ежедневно.



    four. 



    const fs = require('fs');
const testData = JSON.parse(fs.readFileSync('./testData.json', 'utf8'));
describe('some block', ()=>{
    it('should do something', ()=>{
     someTest(testData);
  })
})


    What's wrong here
    Загрузка тестовых данных вне блоков теста.

    На первый взгляд кажется, что всё равно, где читать тестовые данные — в блоке describe, it или в самом модуле. На второй тоже. Но представьте, что у вас сотни тестов, и во многих из них используются тяжёлые данные. Если вы их грузите вне теста, то это приведёт к тому, что все тестовые данные будут оставаться в памяти до конца выполнения тестов, и запуск со временем будет потреблять всё больше и больше оперативной памяти — пока не окажется, что тесты больше вообще не запускаются на стандартных рабочих машинах.



    five. 



    const {assert} = require('chai');
const sinon = require('sinon');
classDog{
  // eslint-disable-next-line class-methods-use-this
  say()
  {
    return'Wow';
  }
}
describe('stubsEverywhere', ()=>{
  before(()=>{
    sinon.stub(Dog.prototype, 'say').callsFake(()=>{
      return'meow';
    });
  });
  it('should say meow', ()=>{
    const dog = new Dog();
    assert.equal(dog.say(), 'meow', 'dog should say "meow!"');
  });
});


    What's wrong here
    Код фактически заменён стабами.

    Наверняка вы сразу увидели эту смешную ошибку. В реальном коде это, конечно, не настолько очевидно — но я видел код, который был обвешан стабами настолько, что вообще ничего не тестировал.



    6 



    const sinon = require('sinon');
const {assert} = require('chai');
classWidget{
  fetch()
  {}
  loadData()
  {
    this.fetch();
  }
}
if (!sinon.sandbox || !sinon.sandbox.stub) {
  sinon.sandbox = sinon.createSandbox();
}
describe('My widget', () => {
  it('is awesome', () => {
    const widget = new Widget();
    widget.fetch = sinon.sandbox.stub().returns({ one: 1, two: 2 });
    widget.loadData();
    assert.isTrue(widget.fetch.called);
  });
});


    What's wrong here
    Зависимость между тестами.

    С первого взгляда понятно, что здесь забыли написать 

      afterEach(() => {
    sinon.sandbox.restore();
  });


    Но проблема не только в этом, а в том, что для всех тестов используется один и тот же sandbox. И очень легко таким образом смутировать среду выполнения тестов таким образом, что они начнут зависеть друг от друга. После этого тесты начнут выполняться только в определённом порядке, и вообще непонятно что тестировать.

    К счастью, sinon.sandbox в какой-то момент был объявлен устаревшим и выпилен, так что с такой проблемой вы можете столкнуться только на легаси проекте — но есть огромное множество других способов смутировать среду выполнения тестов таким образом, что потом будет мучительно больно расследовать, какой код виновен в некорректном поведении. На хабре, кстати, недавно был пост про какой-то шаблон вроде «Ice Factory» — это не панацея, но иногда помогает в таких случаях.




    7. Huge test data in the test file



    Very often I saw how huge JSON files, and even XML, lay right in the test. I think, obviously, why it is not worth doing - it becomes painful to watch it, edit it, and any IDE will not tell you for such a thank you. If you have large test data, take it out of the test file.

    eight. 


    const {assert} = require('chai');
const crypto = require('crypto');
describe('extraTests', ()=>{
  it('should generate unique bytes', ()=>{
    const arr = [];
    for (let i = 0; i < 1000; i++)
    {
      const value = crypto.randomBytes(256);
      arr.push(value);
    }
    const unique = arr.filter((el, index)=>arr.indexOf(el) === index);
    assert.equal(arr.length, unique.length, 'Data is not random enough!');
  });
});


    What's wrong here
    Лишние тесты.

    В данном случае разработчик был очень обеспокоен тем, что его уникальные идентификаторы будут уникальными, поэтому написал на это проверку. В целом понятное стремление — но лучше прочитать документацию или прогнать такой тест несколько раз, не добавляя его в проект. Запуск его в каждом билде не имеет никакого смысла.

    Ну и завязка на случайные значения в тесте — сама по себе является отличным способом выстрелить себе в ногу, сделав нестабильный тест на пустом месте.



    9. Lack of mocks


    It is much easier to run tests with a live base and sotalnogo services, and drive tests for them.
    But sooner or later it will come around - data deletion tests will be performed on the product base, they will start to fall due to a non-functioning partner service, or your CI simply will not have a base on which to drive them away. In general, the item is quite holivarny, but as a rule - if you can emulate external services, then it is better to do it.

    eleven. 


    const {assert} = require('chai');
classCustomErrorextendsError{
}
functionmytestFunction()
{
  thrownew CustomError('important message');
}
describe('badCompare', ()=>{
  it('should throw only my custom errors', ()=>{
    let errorHappened = false;
    try {
      mytestFunction();
    }
    catch (err)
    {
      errorHappened = true;
      assert.isTrue(err instanceof CustomError);
    }
    assert.isTrue(errorHappened);
  });
});


    What's wrong here
    Усложнённая отладка ошибок.

    Всё неплохо, но есть одна проблема — если тест вдруг упал, то вы увидите ошибку вида

    1) badCompare
    should throw only my custom errors:

    AssertionError: expected false to be true
    + expected - actual

    -false
    +true

    at Context.it (test/011_badCompare/test.js:23:14)

    Дальше, чтобы понять, что за ошибка собственно случилась — вам придётся переписывать тест. Так что в случае неожиданной ошибки — постарайтесь, чтобы тест рассказал о ней, а не только сам факт того, что она произошла.



    12. 


    const {assert} = require('chai');
functionsomeVeryBigFunc1()
{
  return1; // imagine a tonn of code here
}
functionsomeVeryBigFunc2()
{
  return2; // imagine a tonn of code here
}
describe('all Before Tests', ()=>{
  let res1;
  let res2;
  before(async ()=>{
    res1 = await someVeryBigFunc1();
    res2 = await someVeryBigFunc2();
  });
  it('should return 1', ()=>{
    assert.equal(res1, 1);
  });
  it('should return 2', ()=>{
    assert.equal(res2, 2);
  });
});


    What's wrong here
    Всё в блоке before.

    Казалось бы, классный подход сделать все операции в блоке `before`, и таким образом оставить внутри `it` только проверки.
    На самом деле нет.
    Потому что в этом случае возникает каша, в которой нельзя ни понять время реального выполнения тестов, ни причину падения, ни то, что относится к одному тесту, а что к другому.
    Так что вся работа теста (кроме стандартных инициализаций) должна выполняться внутри самого теста.



    13. 


    const {assert} = require('chai');
const moment = require('moment');
functionsomeDateBasedFunction(date)
{
  if (moment().isAfter(date))
  {
    return0;
  }
  return1;
}
describe('useFutureDate', ()=>{
  it('should return 0 for passed date', ()=>{
    const pastDate = moment('2010-01-01');
    assert.equal(someDateBasedFunction(pastDate), 0);
  });
  it('should return 1 for future date', ()=>{
    const itWillAlwaysBeInFuture = moment('2030-01-01');
    assert.equal(someDateBasedFunction(itWillAlwaysBeInFuture), 1);
  });
});


    What's wrong here
    Завязка на даты.

    Тоже казалось бы очевидная ошибка — но тоже периодически возникает у уставших разработчиков, которые уже считают, что завтра никогда не наступит. И билд который отлично собирался вчера, внезапно падает сегодня.

    Помните, что любая дата наступит рано или поздно — так что или используйте эмуляцию времени штуками вроде `sinon.fakeTimers`, или хотя бы ставьте отдалённые даты вроде 2050 года — пускай голова болит у ваших потомков...



    14. 



    describe('dynamicRequires', ()=>{
  it('should return english locale', ()=>{
    // HACK :// Some people mutate locale in tests to chinese so I will require moment here// eslint-disable-next-line global-requireconst moment = require('moment');
    const someDate = moment('2010-01-01').format('MMMM');
    assert.equal(someDate, 'January');
  });
});


    What's wrong here
    Динамическая подгрузка модулей.

    Если у вас стоит Eslint, то вы наверняка уже запретили динамические зависимости. Или нет.
    Часто вижу, что разработчики стараются подгружать библиотеки или различные модули прямо внутри тестов. При этом они в целом знают, как работает `require` — но предпочитают иллюзию того, что им будто бы дадут чистый модуль, который никто пока что не смутировал.
    Такое предположение опасно тем, что загрузка дополнительных модулей во время тестов происходит медленнее, и опять же приводит к большему количеству неопределённого поведения.



    15. 


    functionsomeComplexFunc()
{
  // Imagine a piece of really strange code herereturn1;
}
describe('cryptic', ()=>{
  it('success', ()=>{
    const result = someComplexFunc();
    assert.equal(result, 1);
  });
  it('should not fail', ()=>{
    const result = someComplexFunc();
    assert.equal(result, 1);
  });
  it('is right', ()=>{
    const result = someComplexFunc();
    assert.equal(result, 1);
  });
  it('makes no difference for solar system', ()=>{
    const result = someComplexFunc();
    assert.equal(result, 1);
  });
});


    What's wrong here
    Непонятные названия тестов.

    Наверное, вы устали от очевидны вещей, да? Но всё равно придётся о ней сказать потому что многие не утруждаются написанием понятных названий для тестов — и в результате понять, что делает тот или иной тест, можно только после долгих исследований.



    sixteen. 


    const {assert} = require('chai');
constPromise = require('bluebird');
functionsomeTomeoutingFunction()
{
  thrownewPromise.TimeoutError();
}
describe('no Error check', ()=>{
  it('should throw error', async ()=>{
    let timedOut = false;
    try {
      await someTomeoutingFunction();
    }
    catch (err)
    {
      timedOut = true;
    }
    assert.equal(timedOut, true);
  });
});


    What's wrong here
    Отсутствие проверки выброшенной ошибки.

    Часто нужно проверить, что в каком-то случае функция выкидывает ошибку. Но всегда нужно проверять, те ли это дроиды, которых мы ищем — поскольку внезапно может оказаться, что ошибка была выкинута другая, в другом месте и по другим причинам...



    17 



    functionsomeBadFunc()
{
  thrownewError('I am just wrong!');
}
describe.skip('skipped test', ()=>{
  it('should be fine', ()=>{
    someBadFunc();
  });
});


    What's wrong here
    Отключенные тесты.

    Конечно, всегда может появиться ситуация, когда код уже много раз протестирован руками, нужно срочно его катить, а тест почему-то не работает. Например, из-за не очевидной завязки на другой тест, о которой я писал ранее. И тест отключают. И это нормально. Не нормально — не поставить мгновенно задачу на то, чтобы включить тест обратно. Если это не сделать, то количество отключенных тестов будет плодиться, а их код — постоянно устаревать. Пока не останется единственный вариант — проявить милосердие и выкинуть все эти тесты нафиг, поскольку быстрее написать их заново, чем разбираться в ошибках.



    Here is such a selection came out. All these tests are well tested, but they are broken by design. Add your options in the comments, or in the repository that I made to collect such errors.
    Tags:

    Also popular now: