search

RefactoringCombos / ArlosCommitNotation

A notation for small commits messages that show the risk involved in each step
309 stars 30 forks source link

Consider allowing `r` when test coverage is truly comprehensive. #35

Open JayBazuzi opened 2 years ago

JayBazuzi commented 2 years ago

It is possible to use tests to eliminate even unknown risks in some cases.

For example, TeamCity's Kotlin DSL is a program that takes 0 inputs, consistently producing the same outputs (some XML files) each time it is run. Golden Master / Approval Testing the generated XML is sufficient to guarantee that a refactoring did not change anything: if the generated XML is unchanged, the program is unchanged.

When we say to use R for Test-supported refactoring, we assume that the test coverage is incomplete, but in the above example the test coverage is complete.

This is an even higher level of proof than we get with almost all automated refactoring tools.

JayBazuzi commented 2 years ago

This may be too nuanced to add to ACN. People may interpret it as "I have a lot of tests, I can use r."

For example, in the Gilded Rose kata, if you run the scenario for 30 days and log everything and approve that (https://github.com/emilybache/GildedRose-Refactoring-Kata/blob/main/csharp/ApprovalTest.cs#L25) - that is pretty good, and probably qualifies for R but not r.

Maybe "r for truly comprehensive tests" gets added in a separate document, so it doesn't lead newbies astray.

isidore commented 2 years ago

Simpler example:

double pi() { ... }

you could write a test to capture the result, and then you would have all possible paths and results.

because

  1. ALL possible inputs are tested
  2. The results are deterministic, always produce the same result for the input
  3. There are no unknown side effects / All known side effects are captured & tested

refactoring here would satisfy the r prefix