Slide 1

Slide 1 text

Adding Tests to Untestable Legacy Code HEISENBUG MOSCOW | OCT 5, 2021 @afilina

Slide 2

Slide 2 text

"We need tests to refactor, but we need to refactor the code to make it testable." Only a problem if we write the wrong tests at the wrong time of the refactoring process.

Slide 3

Slide 3 text

Anna Filina • Coding since 1997 • VB, PHP, Java, Ruby, C#, etc. • Legacy archaeology • Test automation • Public speaking • Mentorship • Twitter legacy woes • YouTube videos

Slide 4

Slide 4 text

No Tests Hard to justify a testing spree.

Slide 5

Slide 5 text

Always write tests with other work. Don't make it a standalone task.

Slide 6

Slide 6 text

• Bug report. • New feature. • Refactoring. • Build coverage over time.

Slide 7

Slide 7 text

Not Unit-Testable

Slide 8

Slide 8 text

• Bootstraps the framework. • Hits the database. • Call external APIs. • Requires extensive mocking. • Can't mock. The code is not untestable, just not unit-testable.

Slide 9

Slide 9 text

Unit tests end up being convoluted and not useful. Natural response: refactor to make code more testable.

Slide 10

Slide 10 text

Unit tests are sensitive to structure changes. Say you test a class.

Slide 11

Slide 11 text

That class might not exist after refactoring, so you throw the test away. What was the point?

Slide 12

Slide 12 text

• Break up class. • Extract method. • Change method signature. • Reorganize dependencies. Such changes will invalidate unit tests. This defies the purpose of regression tests.

Slide 13

Slide 13 text

You need to keep the same test for the "before" and "after" code, so don't write unit tests yet.

Slide 14

Slide 14 text

Start with acceptance tests. Interact with the whole system using request/response.

Slide 15

Slide 15 text

ASP Classic Tests like that don't even care in which language the application is written.

Slide 16

Slide 16 text

PHP I used this approach to migrate an application from ASP Classic to PHP.

Slide 17

Slide 17 text

Scenario: User can subscribe with a credit card Given I selected a subscription level When I enter valid credit card details Then I should see a payment receipt For acceptance tests, I use any framework that supports Gherkin.

Slide 18

Slide 18 text

• PHP: Behat • C#: SpecFlow • Java: Cucumber There are even frameworks for testing mobile and desktop applications.

Slide 19

Slide 19 text

Slower but fewer You'll still use unit tests for the fine-grained logic and implementation details.

Slide 20

Slide 20 text

Unit Tests

Slide 21

Slide 21 text

Acceptance tests will pass with both the original and refactored code.

Slide 22

Slide 22 text

Any time you refactor, write new unit tests for the new code.

Slide 23

Slide 23 text

class ProductController extends AbstractController { public function search() { //... return $this->render("products/search.html.twig", $products); } } To test this controller, you need to bring in the entire framework.

Slide 24

Slide 24 text

class ProductController { //... public function __construct(Templating $templating) { $this->templating = $templating; } public function search() { //... return $this->templating->render("products/search.html.twig", $products); } } Don't extend. Inject dependencies so you can mock them.

Slide 25

Slide 25 text

• Write high-level tests that survive refactoring. • Write unit tests for new or refactored code. This is the plan for an application that has no tests.

Slide 26

Slide 26 text

Broken Tests Some applications already have tests, but they might be broken or of poor quality.

Slide 27

Slide 27 text

• Didn't compile. • Most failed due to DB changes. • After fixes, many kept failing. • After review, most were wrong or redundant. This was an application when one small feature had 300 acceptance tests, but we couldn't use them.

Slide 28

Slide 28 text

• Checking that something did not happen. • Comparing large DB dump after execution. • Many unnecessary permutations. • Many scenarios still untested. Example: e-mail not sent, but this can happen due to a crash.

Slide 29

Slide 29 text

Afraid to throw code away The alternative can be even more costly.

Slide 30

Slide 30 text

• Fixing estimated at 6 months. • The tests would still be unmaintainable. • Many scenarios would still require new tests.

Slide 31

Slide 31 text

• 46 new tests. • All scenarios covered. • No redundancies. • Easy to read and to maintain.

Slide 32

Slide 32 text

Scenario: TLA1217-A01 Given File is present with records for DE-BER / 2019-01-01 Given Table import_log is empty Given Table country contains: | id | name | area | population | timezone_offset | timezone_offset_dst | created_date | updated_date | | 1 | Germany | 357386 | 83000000 | 1 | 2 | 2019-01-01 | 2019-01-01 | | 2 | Lithuania | 357386 | 83000000 | 1 | 2 | 2019-01-01 | 2019-01-01 | | 3 | Argentina | 357386 | 83000000 | 1 | 2 | 2019-01-01 | 2019-01-01 | Given Table city contains: | id | country_id | area | population | elevation | latitude | longitude | geocode | created_date | updated_date | name | code | | 1 | 1 | 891.7 | 3748148 | 34 | 52.516667 | 13.388889 | DE3 | 2019-01-01 | 2019-01-01 | Berlin | DE-BER | | 2 | 1 | 891.7 | 3748148 | 34 | 52.516667 | 13.388889 | DE3 | 2019-01-01 | 2019-01-01 | Vilnius | LT-VLN | | 3 | 1 | 891.7 | 3748148 | 34 | 52.516667 | 13.388889 | DE3 | 2019-01-01 | 2019-01-01 | Buenos Aires | AR-BAI | Given Table weather_line contains: | id | min_temp | max_temp | rainfall | evaporation | sunshine | wind_gust_dir | wind_gust_speed | wind_dir_9am | wind_dir3_pm | wind_speed9_am | wind_speed3_pm | humidity9_am | humidity3_pm | pressure9_am | pressure3_pm | cloud9_am | cloud3_pm | temp9_am | temp3_pm | rain_today | risk_mm | 1 | 8 | 24.3 | 0 | 3.4 | 6.3 | NW | 30 | SW | NW | 6 | 20 | 68 | 29 | 1019.7 | 1015 | 7 | 7 | 14.4 | 23.6 | No | 3.6 | 2 | 14 | 26.9 | 3.6 | 4.4 | 9.7 | ENE | 39 | E | W | 4 | 17 | 80 | 36 | 1012.4 | 1008.4 | 5 | 3 | 17.5 | 25.7 | Yes | 3.6 | 3 | 13.7 | 23.4 | 3.6 | 5.8 | 3.3 | NW | 85 | N | NNE | 6 | 6 | 82 | 69 | 1009.5 | 1007.2 | 8 | 7 | 15.4 | 20.2 | Yes | 39.8 | 4 | 13.3 | 15.5 | 39.8 | 7.2 | 9.1 | NW | 54 | WNW | W | 30 | 24 | 62 | 56 | 1005.5 | 1007 | 2 | 7 | 13.5 | 14.1 | Yes | 2.8 | 5 | 7.6 | 16.1 | 2.8 | 5.6 | 10.6 | SSE | 50 | SSE | ESE | 20 | 28 | 68 | 49 | 1018.3 | 1018.5 | 7 | 7 | 11.1 | 15.4 | Yes | 0 | 6 | 6.2 | 16.9 | 0 | 5.8 | 8.2 | SE | 44 | SE | E | 20 | 24 | 70 | 57 | 1023.8 | 1021.7 | 7 | 5 | 10.9 | 14.8 | No | 0.2 | 7 | 6.1 | 18.2 | 0.2 | 4.2 | 8.4 | SE | 43 | SE | ESE | 19 | 26 | 63 | 47 | 1024.6 | 1022.2 | 4 | 6 | 12.4 | 17.3 | No | 0 | 8 | 8.3 | 17 | 0 | 5.6 | 4.6 | E | 41 | SE | E | 11 | 24 | 65 | 57 | 1026.2 | 1024.2 | 6 | 7 | 12.1 | 15.5 | No | 0 | 9 | 8.8 | 19.5 | 0 | 4 | 4.1 | S | 48 | E | ENE | 19 | 17 | 70 | 48 | 1026.1 | 1022.7 | 7 | 7 | 14.1 | 18.9 | No | 16.2 | 10 | 8.4 | 22.8 | 16.2 | 5.4 | 7.7 | E | 31 | S | ESE | 7 | 6 | 82 | 32 | 1024.1 | 1020.7 | 7 | 1 | 13.3 | 21.7 | Yes | 0 | 11 | 9.1 | 25.2 | 0 | 4.2 | 11.9 | N | 30 | SE | NW | 6 | 9 | 74 | 34 | 1024.4 | 1021.1 | 1 | 2 | 14.6 | 24 | No | 0.2 | 12 | 8.5 | 27.3 | 0.2 | 7.2 | 12.5 | E | 41 | E | NW | 2 | 15 | 54 | 35 | 1023.8 | 1019.9 | 0 | 3 | 16.8 | 26 | No | 0 | 13 | 10.1 | 27.9 | 0 | 7.2 | 13 | WNW | 30 | S | NW | 6 | 7 | 62 | 29 | 1022 | 1017.1 | 0 | 1 | 17 | 27.1 | No | 0 | 14 | 12.1 | 30.9 | 0 | 6.2 | 12.4 | NW | 44 | WNW | W | 7 | 20 | 67 | 20 | 1017.3 | 1013.1 | 1 | 4 | 19.7 | 30.7 | No | 0 | 15 | 10.1 | 31.2 | 0 | 8.8 | 13.1 | NW | 41 | S | W | 6 | 20 | 45 | 16 | 1018.2 | 1013.7 | 0 | 1 | 18.7 | 30.4 | No | 0 When I execute TLA1217 Then Table import_summary contains: | id | created_date | status | rows_imported | rows_skipped | | 1 | 2019-01-01 | success | 7 | 5 | Then Table weather_line contains: | id | min_temp | max_temp | rainfall | evaporation | sunshine | wind_gust_dir | wind_gust_speed | wind_dir_9am | wind_dir3_pm | wind_speed9_am | wind_speed3_pm | humidity9_am | humidity3_pm | pressure9_am | pressure3_pm | cloud9_am | cloud3_pm | temp9_am | temp3_pm | rain_today | risk_mm | 1 | 8 | 24.3 | 0 | 3.4 | 6.3 | NW | 30 | SW | NW | 6 | 20 | 68 | 29 | 1019.7 | 1015 | 7 | 7 | 14.4 | 23.6 | No | 3.6 | 2 | 14 | 26.9 | 3.6 | 4.4 | 9.7 | ENE | 39 | E | W | 4 | 17 | 80 | 36 | 1012.4 | 1008.4 | 5 | 3 | 17.5 | 25.7 | Yes | 3.6 | 3 | 13.7 | 23.4 | 3.6 | 5.8 | 3.3 | NW | 85 | N | NNE | 6 | 6 | 82 | 69 | 1009.5 | 1007.2 | 8 | 7 | 15.4 | 20.2 | Yes | 39.8 | 4 | 13.3 | 15.5 | 39.8 | 7.2 | 9.1 | NW | 54 | WNW | W | 30 | 24 | 62 | 56 | 1005.5 | 1007 | 2 | 7 | 13.5 | 14.1 | Yes | 2.8 | 5 | 7.6 | 16.1 | 2.8 | 5.6 | 10.6 | SSE | 50 | SSE | ESE | 20 | 28 | 68 | 49 | 1018.3 | 1018.5 | 7 | 7 | 11.1 | 15.4 | Yes | 0 | 6 | 6.2 | 16.9 | 0 | 5.8 | 8.2 | SE | 44 | SE | E | 20 | 24 | 70 | 57 | 1023.8 | 1021.7 | 7 | 5 | 10.9 | 14.8 | No | 0.2 | 7 | 6.1 | 18.2 | 0.2 | 4.2 | 8.4 | SE | 43 | SE | ESE | 19 | 26 | 63 | 47 | 1024.6 | 1022.2 | 4 | 6 | 12.4 | 17.3 | No | 0 | 8 | 8.3 | 17 | 0 | 5.6 | 4.6 | E | 41 | SE | E | 11 | 24 | 65 | 57 | 1026.2 | 1024.2 | 6 | 7 | 12.1 | 15.5 | No | 0 | 9 | 8.8 | 19.5 | 0 | 4 | 4.1 | S | 48 | E | ENE | 19 | 17 | 70 | 48 | 1026.1 | 1022.7 | 7 | 7 | 14.1 | 18.9 | No | 16.2 | 10 | 8.4 | 22.8 | 16.2 | 5.4 | 7.7 | E | 31 | S | ESE | 7 | 6 | 82 | 32 | 1024.1 | 1020.7 | 7 | 1 | 13.3 | 21.7 | Yes | 0 | 11 | 9.1 | 25.2 | 0 | 4.2 | 11.9 | N | 30 | SE | NW | 6 | 9 | 74 | 34 | 1024.4 | 1021.1 | 1 | 2 | 14.6 | 24 | No | 0.2 | 12 | 8.5 | 27.3 | 0.2 | 7.2 | 12.5 | E | 41 | E | NW | 2 | 15 | 54 | 35 | 1023.8 | 1019.9 | 0 | 3 | 16.8 | 26 | No | 0 | 13 | 10.1 | 27.9 | 0 | 7.2 | 13 | WNW | 30 | S | NW | 6 | 7 | 62 | 29 | 1022 | 1017.1 | 0 | 1 | 17 | 27.1 | No | 0 | 14 | 12.1 | 30.9 | 0 | 6.2 | 12.4 | NW | 44 | WNW | W | 7 | 20 | 67 | 20 | 1017.3 | 1013.1 | 1 | 4 | 19.7 | 30.7 | No | 0 | 15 | 10.1 | 31.2 | 0 | 8.8 | 13.1 | NW | 41 | S | W | 6 | 20 | 45 | 16 | 1018.2 | 1013.7 | 0 | 1 | 18.7 | 30.4 | No | 0 | 16 | 12.4 | 32.1 | 0 | 8.4 | 11.1 | E | 46 | SE | WSW | 7 | 9 | 70 | 22 | 1017.9 | 1012.8 | 0 | 3 | 19.1 | 30.7 | No | 0 | 17 | 13.8 | 31.2 | 0 | 7.2 | 8.4 | ESE | 44 | WSW | W | 6 | 19 | 72 | 23 | 1014.4 | 1009.8 | 7 | 6 | 20.2 | 29.8 | No | 1.2 | 18 | 11.7 | 30 | 1.2 | 7.2 | 10.1 | S | 52 | SW | NE | 6 | 11 | 59 | 26 | 1016.4 | 1013 | 1 | 5 | 20.1 | 28.6 | Yes | 0.6 | 19 | 12.4 | 32.3 | 0.6 | 7.4 | 13 | E | 39 | NNE | W | 4 | 17 | 60 | 25 | 1017.1 | 1013.3 | 1 | 3 | 20.2 | 31.2 | No | 0 | 20 | 15.6 | 33.4 | 0 | 8 | 10.4 | NE | 33 | NNW | NNW | 2 | 13 | 61 | 27 | 1018.5 | 1013.7 | 0 | 1 | 22.8 | 32 | No | 0 | 21 | 15.3 | 33.4 | 0 | 8.8 | 9.5 | WNW | 59 | N | NW | 2 | 31 | 60 | 26 | 1012.4 | 1006.5 | 1 | 5 | 22.2 | 32.8 | No | 0.4 | 22 | 16.4 | 19.4 | 0.4 | 9.2 | 0 | E | 26 | ENE | E | 6 | 11 | 88 | 72 | 1010.7 | 1008.9 | 8 | 8 | 16.5 | 18.3 | No | 25.8 Then Table import_log contains: | id | created_date | type | message | | 1 | 2019-01-01 | warning | Invalid city code "FR-PAR" | | 2 | 2019-01-01 | warning | Invalid city code "FR-PAR" | | 3 | 2019-01-01 | warning | Invalid city code "FR-PAR" | | 4 | 2019-01-01 | warning | Invalid city code "FR-PAR" | | 5 | 2019-01-01 | warning | Invalid city code "FR-PAR" | Example of existing tests: basically DB dump in and DB dump out.

Slide 33

Slide 33 text

Scenario: Add warning for invalid city Given city exists for RU-MSK And file contains entry for RU-MSK / 2021-01-01 And file contains entry for CA-MTL / 2021-01-01 When I execute Import Daily Weather Then 1 weather entry should have been imported And warning Invalid city code "CA-MTL" should be logged New tests were readable and maintainable. Only 46 of them, and they covered all use cases.

Slide 34

Slide 34 text

• Will the tests survive refactoring? • Effort of fixing vs rewriting tests. • Do the tests give you confidence? • Can you maintain these tests? Based on those answers, you'll know whether to salvage or rewrite the tests.

Slide 35

Slide 35 text

Thank you! @afilina