Срокът за четвърта задача свърши. Решенията са проверени, тестовете са пуснати и може да видите нашите тестове тук: https://github.com/fmi/ruby-homework/blob/master/tasks/04/original_solution.rb

В тази тема ще "изсипя" най-често срещаните проблеми. Тези, които предадоха до 23:30 в четвъртък също са получили и inline коментари от мен.

Всички прилики с решения на студенти са напълно случайни.

0. Променливи извън тестовете

describe '#>' do
  bigger = Version.new("1.0.1.6")
  smaller = Version.new("1.0.0.8")
  dup_of_smaller = Version.new(smaller)

  it 'returns false when called with <smaller> and <bigger> argument' do
    expect(smaller).to be < bigger
  end
end

Горното е лошо, защото ако един тест промени някой от обектите, следващите тестове също ще фейлнат. Ето пример:

a_string = 'test'

it 'it contains test' do
  a_string.sub!('test', '')
end

it 'is now broken' do
  expect(a_string).to eq 'test'
end

Освен това, всякакви mock-ове и stub-ове няма да важат за тези променливи.

1. let-овете на смъртта

let(:square_numbers) { [1, 4, 9, 16] }
let(:version_square) { Version.new(square_numbers.join('.')) }
let(:major_range) { Version::Range.new('1', '2') }
let(:invalid_version_msg) { 'Invalid version string' }
let(:snapshot_version) { '1.0-SNAPSHOT' }
let(:beta_version) { '1.0-BETA' }
let(:negative_version) { '1.-1' }
let(:positive_version) { '1.+1' }
let(:underscore_version) { '1.1_000' }
let(:missing_major_version) { '.3' }
let(:missing_minor_version) { '0..3' }
let(:missing_build_version) { '3.2.15.' }

# 250 реда по-късно

expect(major_range.include?('2')).to eq false

Всеки път, когато ми се наложи да скролна нагоре, за да разбера един тест, значи нещо в този тест е страшно объркано. Когато трябва да скролна 250 реда, за да разбера един ред - дяволчето на едното ми рамо ме кара да вземам точки. :)

Защо не директно expect(Version::Range.new('1', '2').include?('2')).to eq false или, по-добре, expect(Version::Range.new('1', '2')).to include '2'. Няма нужда от контекст, за да се прочете.

2. Описания

it '#>' do
it 'basic versions' do

vs

it 'can compare versions using #>' do
it 'returns components for basic versions' do

Най-важното правило за описанията на it е, че трябва да се чете като изречение. Цялото. Изречението It basic versions не прави никакъв смисъл и не ни е полезно с нищо. То прости версии.

3. Прекалено ситни тестове

Сигурно сте чували от някой, че тестовете трябва да съдържат само един expect. Това, както всичко останало, трябва да приемете с въпросите Защо? и Кога?.

Вземете този код:

describe '#>' do
  context 'when versions have equal length' do
    it 'compares versions and returns true' do
      expect(Version.new('1.3')).to be > Version.new('1.2')
    end

    it 'compares versions and returns false' do
      expect(Version.new('1.2')).to_not be > Version.new('1.3')
    end

    it 'compares equal versions and returns false' do
      expect(Version.new('1.2')).to_not be > Version.new('1.2')
    end
  end

  context 'when versions have different length' do
    it 'compares versions and returns true' do
      expect(Version.new('1.3')).to be > Version.new('1.2')
    end

    it 'compares versions and returns false' do
      expect(Version.new('1.2')).to_not be > Version.new('1.3')
    end

    it 'compares equal versions and returns false' do
      expect(Version.new('1.2')).to_not be > Version.new('1.3.0')
    end
  end
end

Същото може да се напише и така:

describe '#>' do
  it 'compares versions of equal length' do
    expect(Version.new('1.3.1')).to be > Version.new('1.2')
    expect(Version.new('1.2')).to_not be > Version.new('1.3.1')
    expect(Version.new('1.2')).to_not be > Version.new('1.2')
  end

  it 'compares versions of different length' do
    expect(Version.new('1.3.1')).to be > Version.new('1.2')
    expect(Version.new('1.2')).to_not be > Version.new('1.3.1')
    expect(Version.new('1.2')).to_not be > Version.new('1.3.0')
  end
end

Твърдя, че вторият код се разбира по-лесно и е по-компактен.

Напълно ОК е да имате повече от един expect в тест, стига да тестват една и съща логика. В този случай, тази логика е сравнението с >.

4. eq vs match_array - мислете и проверявайте

expect(Version.new('1.3.15.6').components).to match_array [1, 3, 15, 6]

Каква е разликата между eq и match_array? Този въпрос го зададох на поне 4 човека в коментарите из решенията. Никой не ми отговори.

На лекцията в понеделник, Сашо ви каза да не забравяте, че съществува match_array. Това не означава "Използвайте match_array винаги и за всичко". Означава "Запомнете какво прави match_array и го използвайте когато трябва". Кога трябва? Когато е валидно елементите на масива да са в разбъркан ред.

Ако Version.new('1.3.15.6').components върне [6, 15, 3, 1], този тест пак ще мине. Тоест, не си е свършил единствената задача. Когато използвате метод - винаги проверявайте какво прави.

5. Излишни тестове

it 'doesn`t raise error' do
  expect { Version.new(Version.new('1.2.3')) }.to_not raise_error
  expect { Version.new('1.2.3') }.to_not raise_error
  expect { Version.new('1.2.0') }.to_not raise_error
  expect { Version.new('') }.to_not raise_error
  expect { Version.new('0') }.to_not raise_error
  expect { Version.new }.to_not raise_error
end

# ...

it 'converts to string' do
  expect(Version.new('1.1.0').to_s).to eq '1.1'
  expect(Version.new('1.1').to_s).to eq '1.1'
  expect(Version.new('').to_s).to eq ''
  expect(Version.new.to_s).to eq ''
  expect(Version.new(Version.new('1.2.3')).to_s).to eq '1.2.3'
end

Ако се счупи първия тест => ще се счупи и втория. Тоест, можем да махнем първия и пак ще разберем, че има проблем. Първият тест е излишен код, който не ни дава нищо полезно като информация.

Още един пример:

it 'includes first and exludes last' do
    range = Version::Range.new(Version.new('1.1.0'), Version.new('1.2.2'))
    expect(range.to_a).to include('1.1')
    expect(range.to_a).not_to include('1.2.2')
end

it 'lists all versions' do
  expect(Version::Range.new('1.1.0', '1.2.2').to_a).to eq [
    '1.1', '1.1.1', '1.1.2', '1.1.3', '1.1.4', '1.1.5',
    '1.1.6', '1.1.7', '1.1.8', '1.1.9', '1.2', '1.2.1'
  ]
end

Първият тест очевидно е вързан за втория. По същата логика, той е напълно излишен и не допринася с нищо.

6. Излишни променливи и лоши имена

it 'checks if version1 > version2' do
  version1 = Version.new("2.4")
  version2 = Version.new("2.3")
  expect(version1).to be > version2
  version1 = Version.new("0.2.4")
  version2 = Version.new("0.2.3")
  expect(version1).to be > version2
  version1 = Version.new("1.0.0")
  version2 = Version.new("1")
  expect(version1).to be > version2
  version1 = Version.new("0.2.4")
  version2 = Version.new("0.2.4")
  expect(version1).to be >= version2
  version1 = Version.new("2.4")
  version2 = Version.new("2.4")
  expect(version1).to be >= version2
end

Защо това, а не това:

it 'checks if version1 > version2' do
  expect(Version.new("2.4")  ).to be >  Version.new("2.3")
  expect(Version.new("0.2.4")).to be >  Version.new("0.2.3")
  expect(Version.new("1.0.0")).to be >  Version.new("1")
  expect(Version.new("0.2.4")).to be >= Version.new("0.2.4")
  expect(Version.new("2.4")  ).to be >= Version.new("2.4")
end

Кое е по-ясно?

Ето още един пример:

it 'can compare with >=' do
  version1 = Version.new('1.3.1')
  version2 = Version.new('1.3.1')
  version3 = Version.new('1.2.3')
  version4 = Version.new('1.4')
  expect(version1).to be >= version3
  expect(version3).to be >= version1
  expect(version1).to be >= version2
  expect(version4).to be >= version1
end

Тук на мозъкът ми му е трудно да match-ва всички променливи, една по една, накръст.

7. Тестване, че нещо не може да промени обекта

Ако в нашата имплементация на #components нямаше .dup, щеше да се случва това:

version = Version.new('1.2.3')
version.components << 4

version.to_s #=> '1.2.3.4'

Очевидно не искаме да може да става така. Защо? Защото този масив можем да го подадем на друг метод и да го променим несъзнателно. Това ще доведе до изключително трудно намиращи се бъгове.

Това директно може да се напише на тест така:

version = Version.new('1.2.3')
version.components << 4

expect(version.to_s).to eq '1.2.3'

8. To be or not to be

expect(Version.new('2') > Version.new('1')).to be true
expect(Version.new('2') < Version.new('1')).to be false

Да, горното ще работи. Но съобщението, което ще ви даде ако фейлне ще е изоморфно на expected false to be true. Лекцията от понеделник би трябвало да ви е подсетила за следното:

expect(Version.new('2')).to be > Version.new('1')
expect(Version.new('2')).to be < Version.new('1')

Тук, ако фейлне, грешката ще е подобна на expected '2' to be > '1'. Много по-описателно и по-полезно. Разликата между двете е като разликата между if (2 > 1) == true и if 2 > 1. Никога не бихте написали първото, нали?

9. Проверки за тип на обекта при създаване

it 'makes instance with no argument' do
  expect(Version.new).to be_an_instance_of(Version)
end

Създаваме инстанция на класа Version и очакваме тя да е инстанция на класа Version. Почти толкова полезно, колкото expect('a constant string').to eq 'a constant string'. Този тест не прави нищо и не трябва да съществува. Ако някой мисли по друг начин - ще се радвам да обясни защо, за да стигнем до основата на "проблема" :)

10. Скоби

(names.split ' ').first
names.split(' ').first

Кой от горните два реда ви изглежда по-естествено? Втория? Защо тогава това:

expect((range.include? version)).to be true

се вижда по-често в решенията ви от това:

expect(range.include?(version)).to be true

Това е Ruby, не пишете на Haskell или Scheme. Не слагайте скоби на случайни места докато не проработи. include? е метод като всеки друг - нормално е скобите да са около аргументите му.