Когда-то давно, когда я начал писать тесты (на самом деле, не так давно, может быть, несколько лет назад), я был наивным юношей. Я ненавижу баги, поэтому пишу тесты, и я писал их в соответствии со своими ограниченными знаниями на тот момент.

Быть наивным и не совсем в курсе ссылок имеет свою цену. Из каждого отклоненного PR-ревью или регрессионной ошибки я многому научился на своих ошибках и понял, что мне еще многое нужно улучшить. Мне действительно жаль, что мне приходится учиться методом проб и ошибок, но это не должно быть несчастьем для вас!

Скажем, коллеги-разработчики, если вы чувствуете, что ваш тест недостаточно хорош, или ваши PR слишком много раз отклонялись командой контроля качества из-за низкого качества теста, возможно, эта статья окажется вам полезной. Я собираюсь поделиться с вами пятью основными ошибками, которые я допустил при написании тестов, и почему вам следует их избегать.

Перед этим небольшое предупреждение: приведенный ниже пример кода написан на Javascript с использованием Jest в качестве тестовой среды. Я сосредоточен исключительно на Javascript, поэтому я не могу много комментировать другие, не уверен, что его можно применить. Кроме того, это просто упрощенные примеры, они не представляют реальных вариантов использования. Просто чтобы донести суть.

Хорошо. Сразу к примеру. Предположительно, я писал этот класс:

class Account {
  constructor (initialBalance = 0) {
    this.balance = initialBalance
  }

  deposit (money) {
    this.balance += money
  }

  withdraw (money) {
    this.balance -= money
  }
}

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

1. Не делать тест простым

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

const account = new Account()

describe('Account class', () => {
  describe('.deposit', () => {
    test('Should increment the account balance by the amount', () => {
      const increment = 200
      const originalBalance = account.balance
      account.deposit(increment)
      expect(account.balance).toBe(originalBalance + increment)
    })
  })
})

Тест выглядит хорошо, не так ли? У него есть первоначальный баланс, сумма для увеличения, и он утверждает первоначальный баланс плюс приращение. На самом деле, если бы я хотел изменить величину приращения, мне нужно было бы изменить только переменную increment, и тест все равно прошел бы. Вот и все. Супер легко.

Затем появилось новое требование. К каждой вносимой сумме будет добавлено 2% сверх суммы в качестве поощрения (не спрашивайте меня почему, это ПМ…).

  deposit (money) {
    this.balance += (money * 1.02)
  }

Хм, да, ладно. Так что тест будет…

test('Should increment the account balance by the amount plus 2% incentive', () => {
  const increment = 200
  const originalBalance = account.balance
  // PLEASE SEE TEH CODE FOR THE CLASS FOR REFERENCE
  const incrementPlusIncentive = increment * 1.02
  account.deposit(increment)
  expect(account.balance)
    .toBe(originalBalance + incrementPlusIncentive)
})

О боже, что это за чудовище? Моя идея состояла в том, чтобы сделать это ясным, но в итоге я усложнил его. Кроме того, я дублирую логику в коде для теста. Это не правильно.

На практике тестовый код должен только явно указывать, что вы тестируете (ввод -> вывод). Никакого логического кода там быть не должно; он принадлежит коду, который вы тестируете. Вот почему улучшенная версия будет:

test('Should increment the account balance by the amount plus 2% incentive', () => {
  account.deposit(100)
  expect(account.balance).toBe(102)
})

Ну вот. Будь проще. Я вношу 100, мой баланс теперь 102. Это соответствует требованиям? Абсолютно! И это самое главное.

2. Не поддерживать чистое состояние на каждом тесте

Мой следующий квест — написать оставшуюся часть теста. .withdraw это так.

const account = new Account()

describe('Account class', () => {
  describe('.deposit', () => {
    test('Should increment the account balance by the amount plus 2% incentive', () => {
      account.deposit(100)
      expect(account.balance).toBe(102)
    })
  })

  describe('.withdraw', () => {
    test('Should decrement the account balance by the amount', () => {
      account.withdraw(100)
      expect(account.balance).toBe(2)
    })
  })
})

Хм, да, хорошо выглядит. Однако некоторые из вас уже могли это заметить: запах кода присутствует. Почему тесты используют один экземпляр account? Разве это не сделало бы порядок теста важным, когда это не должно быть? Если мы поменяем порядок, он обязательно сломается. Это неправильно.

describe('Account class', () => {
  describe('.deposit', () => {
    test('Should increment the account balance by the amount plus 2% incentive', () => {
      const account = new Account()
      account.deposit(100)
      expect(account.balance).toBe(102)
    })
  })

  describe('.withdraw', () => {
    test('Should decrement the account balance by the amount', () => {
      const account = new Account()
      account.withdraw(100)
      expect(account.balance).toBe(-100)
    })
  })
})

Создание экземпляра account для каждого теста гарантирует, что тест начнется с чистого листа. Его можно изменять сколько угодно, потому что он входит в область действия конкретного теста и не зависит друг от друга. Таким образом, порядок проверки не имеет значения. Скажем, если мы используем средство запуска тестов, которое выполняется параллельно и рандомизирует порядок тестов, оно все равно будет проходить нормально.

И, кстати, есть хелпер beforeEach/afterEach (или setup/teardown), который мы также можем использовать для инициализации и очистки каждого набора тестов, но это довольно сложно объяснить здесь, так что, возможно, для другой статьи.

3. Не правильное утверждение состояния

Затем проект становится большим, очевидно, были какие-то хозяйственные работы, и теперь весь код должен быть прокомментирован, поместить его в правильный файл и еще много чего.

/**
 * Account class.
 */
class Account {
  /**
   * Constructor function.
   * 
   * This sets the initial balance when initializing the instance.
   * 
   * @param {Number} initialBalance 
   */
  constructor (initialBalance = 0) {
    this.balance = initialBalance
  }

  /**
   * Increment the balance by the given sum of the amount.
   * An incentive of 2% of the amount will be added
   * for each deposited amount.
   * 
   * @param {Number} money 
   */
  public deposit (money) {
    this.balance = (money * 1.02)
  }

  /**
   * Decrement the balance by the given amount.
   * 
   * @param {Number} money 
   */
  public withdraw (money) {
    this.balance -= money
  }
}

Хорошо, готово. Ничего плохого я не заметил (или заметил? 😏 Скоро узнаете). Я проверил консоль Jest, и там написано…

Account class
  .deposit
    ✓ Should increment the account balance by the amount plus 2% incentive (5ms)
  .withdraw
    ✓ Should decrement the account balance by the amount

Еще проходит, очевидно. Дух. Принят, PR проверен, сборка CI пройдена, объединена и развернута. Это был веселый понедельник.

…но не совсем. Пользователи кричат, что их баланс сброшен до суммы, которую они вносят. Что случилось? Как это произошло, когда тесты проходят?

Я посмотрел на свой код, посмотрел на тест, вроде ничего страшного. Это первоначальный баланс? Потому что у меня не было теста на это (упс). Итак, я продолжаю и обновляю тест как таковой:

describe('.deposit', () => {
  test('Should increment the account balance by the amount plus 2% incentive', () => {
    const account = new Account(100)
    account.deposit(100)
    expect(account.balance).toBe(202)
  })
})

Глядь, не только пользователи, Jest теперь тоже кричат ​​(?)

● Account class › .deposit › Should increment the account balance 
  by the amount plus 2% incentive
expect(received).toBe(expected) // Object.is equality
  Expected: 202
  Received: 102
    11 |
    12 |       account.deposit(100)
  > 13 |       expect(account.balance).toBe(202)
       |                               ^
    14 |     })
    15 |   })
    16 |
at Object.toBe (__tests__/index.test.js:13:31)

Баг появился! Именно об этом сообщают пользователи. Теперь тест фактически провален. Посмотрев на код (а вы можете сравнить с кодом из начала этого раздела), я заметил одну крошечную ошибку:

deposit (money) {
  // The plus was missing 🤮
  this.balance += (money * 1.02)
}

Ага, вот так. Якобы безобидный рефакторинг привел к багу, вероятно, плюс убрали случайно. И тест его не поймал. Я должен был написать это правильно в первую очередь.

Если код предназначен для накопления значений (а не для присвоения значений), его нужно протестировать таким образом, чтобы предыдущее значение суммировалось с заданным значением. Предыдущее утверждение было своего рода неполным, потому что оно просто проверяет присвоение значения.

// 🤔 
describe('.deposit ❌', () => {
  test('Should increment the account balance by the amount plus 2% incentive', () => {
    const account = new Account() //... What's the previous value?
    account.deposit(100) // Amount is 100
    expect(account.balance).toBe(102) // Final value is 102...?
  })
})
// 😎
describe('.deposit ✅', () => {
  test('Should increment the account balance by the amount plus 2% incentive', () => {
    const account = new Account(100) // Previous value is 100
    account.deposit(100) // Amount is 100
    expect(account.balance).toBe(202) // Final value is 202
  })
})

Чтобы связать себя узами брака, необходимо также протестировать функцию конструктора. Это гарантирует, что часть создания экземпляра покрывается должным образом (возможно, если функция-конструктор имеет некоторую логику, она также может быть утверждена).

describe('constructor', () => {
  test('Should set the initial balance when instantiated', () => {
    const account = new Account(100)
    expect(account.balance).toBe(100)
  })
})

Возможно, этот раздел довольно специфичен, но суть в том, что всегда проверяйте весь поток состояния (до/после, ввод-вывод), а не только частично. По крайней мере, это то, что я узнал.

4. Неправильное структурирование тестов

Я получил сообщение от команды обеспечения качества, что я не улавливал должным образом крайние случаи. Значения в .deposit могут быть любыми, и ошибка недостаточно интуитивна.

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

Отлично. Код .deposit теперь выглядит так:

/**
  * Increment the balance by the given sum of the amount.
  * An incentive of 2% of the amount will be added
  * for each deposited amount.
  * Only number is allowed, otherwise an error is thrown.
  * Also, the number should be greater than 0.
  * 
  * @param {Number[]} ...args 
  */
deposit (...args) {
  if (args.length === 0) {
    throw new Error('Please provide at least one argument.')
  }
  const amount = args.reduce((total, value) => {
    const number = parseInt(value, 10)
    if (isNaN(number)) {
      throw new Error('Please specify a number as the argument.')
    }
    if (number <= 0) {
      throw new Error('Please specify a number greater than 0.')
    }
    return total + (number * 1.02)
  })
  this.balance += amount
}

…но тест выглядит не так хорошо:

describe('.deposit', () => {
  test('Should throw an error when no amount is given', () => {
    const account = new Account()
    expect(() => account.deposit())
      .toThrowError('Please provide at least one argument.')
  })
  test('Should throw an error when amount given is not a number', () => {
    const account = new Account()
    expect(() => account.deposit('a', 'b', 'c'))
      .toThrowError('Please specify a number as the argument.')
  })
  test('Should increment the account balance by the sum of the amount plus 2% incentive, only when the amount is greater than 0 otherwise it should throw', () => {
    const account = new Account(100)
    account.deposit(100, 200)
    expect(account.balance).toBe(406)
    // ...but not less than 0!
    expect(() => account.deposit(0, 400, -200))
      .toThrowError('Please specify a number greater than 0.')
  })
})

Ничего себе, последняя часть теста была довольно сложной. Что угодно 🙄. Работа сделана, тесты пройдены.

.deposit
  ✓ Should throw an error when no amount is given (4ms)
  ✓ Should throw an error when amount given is not a number (1ms)
  ✓ Should increment the account balance by the sum of the amount plus 2% incentive, only when the amount is greater than 0 otherwise it should throw (5ms)

Однако команда QA говорит, что тест - это беспорядок! Это трудно понять, и последняя часть теста делает слишком много. В общем, лучше разделить тесты на несколько контекстов, чтобы были слои условий для утверждения, и один тест должен просто делать что-то одно в зависимости от контекста.

Улучшенная версия будет:

describe('.deposit', () => {
  describe('When no argument is provided', () => {
    test('Should throw an error', () => {
      const account = new Account()
      expect(() => account.deposit()).toThrowError('Please provide at least one argument.')
    })
  })
  describe('When the arguments are provided', () => {
    describe('And the arguments are invalid', () => {
      test('Should throw an error', () => {
        const account = new Account()
        expect(() => account.deposit('a', 'b', 'c')).toThrowError('Please specify a number as the argument.')
      })
    })
    describe('And the arguments are valid', () => {
      describe('And the arguments are less than zero', () => {
        test('Should throw an error', () => {
          const account = new Account()
          expect(() => account.deposit(0, 400, -200))
.toThrowError('Please specify a number greater than 0.')
        })
      })
      describe('And the arguments are all more than zero', () => {
        test('Should increment the account balance by the sum of the amount plus 2% incentive', () => {
          const account = new Account(100)
          expect(account.balance).toBe(100)
          account.deposit(100, 200)
          expect(account.balance).toBe(406)
        })
      })
    })
  })
})

Несколько слоев контекста полезны, когда код становится еще более сложным. Легче добавить больше контекстов, когда они уже разделены на такие слои. Например, если бы мне нужно было добавить новую проверку (может быть, должна быть максимальная сумма для депозита) и я должен был добавить тест для этого, я знаю, где разместить их в структуре, красиво и аккуратно.

Порядок слоев в основном мой выбор. Мне нравится видеть пограничные случаи вверху и реальную логику внизу, вроде того, как в коде прописаны охранники (или фактическая проверка).

А вот как это выглядит на выходе Jest:

.deposit
  When no argument is provided
    ✓ Should throw an error (7ms)
  When the arguments are provided
    And the arguments are invalid
      ✓ Should throw an error (2ms)
    And the arguments are valid
      And the arguments are less than zero
        ✓ Should throw an error (2ms)
      And the arguments are all more than zero
        ✓ Should increment the account balance by the sum of the amount plus 2% incentive (2ms)

Теперь я вроде как должен согласиться с командой контроля качества.

5. Не доверяйте используемым вами библиотекам

Заинтересованные стороны говорят, что есть хакеры, которые каким-то образом снимают со счета чужие деньги. Из-за этой проблемы функция .withdraw не будет просто вычитать баланс; он должен пройти через некоторую магию сценария проверки, чтобы узнать, подделывается ли он хакером (я не уверен, как это сделать, это просто пример кода).

/**
 * Decrement the balance by the given amount.
 * It is now using a validator from backend
 * which I don't know how it works.
 * 
 * @param {Number} money 
 */
withdraw (money) {
  const currentBalance = this.validateAndWithdraw(money)
  this.balance = currentBalance
}
validateAndWithdraw (money) {
  // This validator might throw an error if the transaction is invalid!!!
  return superDuperValidatorFromBackend(money)
}

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

describe('.withdraw', () => {
  describe('Given a valid withdrawal', () => {
    test('Should set the balance after withdrawal', () => {
      const account = new Account(300)
      // Override this function to avoid
      // having to actually request from backend.
      // It should just return the balance without any error thrown.
      jest
        .spyOn(account, 'validateAndWithdraw')
        .mockImplementationOnce(() => 200)
      expect(() => account.withdraw(100)).not.toThrow()
      expect(account.validateAndWithdraw).toHaveBeenCalledWith(100)
      expect(account.balance).toBe(200)
    })
  })
})

Я добавил туда not.toThrow(), чтобы знать, что когда я вызываю функцию .withdraw, не возникает ошибки, потому что я издевался над ней. Правильно? Правильно?

Неверно. — команда контроля качества

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

…но как вы можете быть так уверены? — Мой неуклюжий навык тестирования

Всегда можно проверить репозиторий Jest, исходный код и то, как они его тестируют, и проходит ли он проверку. Там может быть даже точный код, кто знает. Дело в том, что я должен доверять библиотекам, которые использую, и они несут ответственность за тестирование, чтобы убедиться, что их код работает, а не мой. Вместо этого мой тест должен сосредоточиться на фактической логике моего кода.

describe('.withdraw', () => {
  describe('Given a valid withdrawal', () => {
    test('Should set the balance after withdrawal', () => {
      const account = new Account(300)
      // Override this function to avoid
      // having to actually request from backend.
      // It should just return the balance without any error thrown.
      jest
        .spyOn(account, 'validateAndWithdraw')
        .mockImplementationOnce(() => 200)
      account.withdraw(100)
      expect(account.validateAndWithdraw).toHaveBeenCalledWith(100)
      expect(account.balance).toBe(200)
    })
  })
})

Вот и все. Разрешена только бизнес-логика.

И это завершает конец моего путешествия, на данный момент. Кто знает, что ждет нас в будущем (ошибки)…

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

Надеюсь, вам понравится читать эту статью, и спасибо!

Первоначально опубликовано на https://dev.to 6 июля 2019 г.