Adding Tests to Untestable Legacy Code

My work consists of improving the code quality in legacy applications, most in the millions of lines. How to approach testing when the code is untestable? Where to begin with an application that has no tests? What to do with existing tests that are either hard to understand or have fallen into disrepair? I will answer all these questions by showing how these problems were solved in real-world projects.

Transcript

1. Adding Tests to Untestable Legacy Code PHPERS SUMMIT 2024 | POZNAŃ, POLAND @afilina@phpc.social | @afilina
2. I Stand With Ukraine “We need tests to refactor, but we need to refactor to make the code testable.”
3. I Stand With Ukraine Anna Filina • Coding since 1997. • PHP, Java, C#, Flash, etc. • Legacy archaeology. • Test automation. • Mentoring. • Filina Consulting.
4. I Stand With Ukraine
5. I Stand With Ukraine No Tests
6. I Stand With Ukraine
7. I Stand With Ukraine Write tests as part of other work.
8. I Stand With Ukraine • Bug report. • New feature. • Refactoring. • Build coverage over time.
9. I Stand With Ukraine Not Unit-Testable
10. I Stand With Ukraine • Might bootstrap the framework. • Hit the database. • Call external APIs. • Require extensive mocking.
11. I Stand With Ukraine Unit tests end up being convoluted and not useful.
12. I Stand With Ukraine
13. I Stand With Ukraine
14. I Stand With Ukraine
15. I Stand With Ukraine • Break up class. • Extract method. • Change method signature. • Reorganize dependencies.
16. I Stand With Ukraine
17. I Stand With Ukraine
18. I Stand With Ukraine ASP Classic
19. I Stand With Ukraine PHP
20. I Stand With Ukraine 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
21. I Stand With Ukraine • Behat • Codeception
22. I Stand With Ukraine Slower but fewer
23. I Stand With Ukraine Unit Tests
24. I Stand With Ukraine
25. I Stand With Ukraine
26. I Stand With Ukraine class ProductController extends AbstractController { public function search() { //... $products = $this->repository->search($criteria); return $this->render("products/search.html.twig", $products); } }
27. I Stand With Ukraine class ProductController { //... public function __construct(Templating $templating) { $this->templating = $templating; } public function search() { //... $products = $this->repository->search($criteria); return $this->templating->render("products/search.html.twig", $products); } }
28. I Stand With Ukraine New Tests • Write characterization that survive refactoring. • Write unit tests for new or refactored code.
29. I Stand With Ukraine Broken Tests
30. I Stand With Ukraine • Didn't compile. • Most failed due to DB changes. • After fixes, many kept failing. • After review, most were wrong or redundant.
31. I Stand With Ukraine • Checking that something did not happen. • Comparing large DB dump after execution. • Many unnecessary permutations. • Many scenarios untested despite 300 tests.
32. I Stand With Ukraine Afraid to throw code away
33. I Stand With Ukraine • Fixing estimated at 6 months. • The tests would still be unmaintainable. • Many scenarios would still require new tests.
34. I Stand With Ukraine Scenario: TLA1 2 1 7 -A0 1 Given File is present with records for DE-BER / 2 0 1 9-0 1 -0 1 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 | 3 5 7 3 86 | 83 0 0 0 0 0 0 | 1 | 2 | 2 0 1 9-0 1 -0 1 | 2 0 1 9-0 1 -0 1 | | 2 | Lithuania | 3 5 7 3 86 | 83 0 0 0 0 0 0 | 1 | 2 | 2 0 1 9-0 1 -0 1 | 2 0 1 9-0 1 -0 1 | | 3 | Argentina | 3 5 7 3 86 | 83 0 0 0 0 0 0 | 1 | 2 | 2 0 1 9-0 1 -0 1 | 2 0 1 9-0 1 -0 1 | Given Table city contains: | id | country_id | area | population | elevation | latitude | longitude | geocode | created_date | updated_date | name | code | | 1 | 1 | 891 .7 | 3 7 4 81 4 8 | 3 4 | 5 2 .5 1 6 6 6 7 | 1 3 .3 88889 | DE3 | 2 0 1 9-0 1 -0 1 | 2 0 1 9-0 1 -0 1 | Berlin | DE-BER | | 2 | 1 | 891 .7 | 3 7 4 81 4 8 | 3 4 | 5 2 .5 1 6 6 6 7 | 1 3 .3 88889 | DE3 | 2 0 1 9-0 1 -0 1 | 2 0 1 9-0 1 -0 1 | Vilnius | LT-VLN | | 3 | 1 | 891 .7 | 3 7 4 81 4 8 | 3 4 | 5 2 .5 1 6 6 6 7 | 1 3 .3 88889 | DE3 | 2 0 1 9-0 1 -0 1 | 2 0 1 9-0 1 -0 1 | 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 | rain_tomorrow | city_code | | 1 | 8 | 2 4 .3 | 0 | 3 .4 | 6 .3 | NW | 3 0 | SW | NW | 6 | 2 0 | 6 8 | 2 9 | 1 0 1 9.7 | 1 0 1 5 | 7 | 7 | 1 4 .4 | 2 3 .6 | No | 3 .6 | Yes | DE-BER | | 2 | 1 4 | 2 6 .9 | 3 .6 | 4 .4 | 9.7 | ENE | 3 9 | E | W | 4 | 1 7 | 80 | 3 6 | 1 0 1 2 .4 | 1 0 0 8.4 | 5 | 3 | 1 7 .5 | 2 5 .7 | Yes | 3 .6 | Yes | DE-BER | | 3 | 1 3 .7 | 2 3 .4 | 3 .6 | 5 .8 | 3 .3 | NW | 85 | N | NNE | 6 | 6 | 82 | 6 9 | 1 0 0 9.5 | 1 0 0 7 .2 | 8 | 7 | 1 5 .4 | 2 0 .2 | Yes | 3 9.8 | Yes | DE-BER | | 4 | 1 3 .3 | 1 5 .5 | 3 9.8 | 7 .2 | 9.1 | NW | 5 4 | WNW | W | 3 0 | 2 4 | 6 2 | 5 6 | 1 0 0 5 .5 | 1 0 0 7 | 2 | 7 | 1 3 .5 | 1 4 .1 | Yes | 2 .8 | Yes | DE-BER | | 5 | 7 .6 | 1 6 .1 | 2 .8 | 5 .6 | 1 0 .6 | SSE | 5 0 | SSE | ESE | 2 0 | 2 8 | 6 8 | 4 9 | 1 0 1 8.3 | 1 0 1 8.5 | 7 | 7 | 1 1 .1 | 1 5 .4 | Yes | 0 | No | DE-BER | | 6 | 6 .2 | 1 6 .9 | 0 | 5 .8 | 8.2 | SE | 4 4 | SE | E | 2 0 | 2 4 | 7 0 | 5 7 | 1 0 2 3 .8 | 1 0 2 1 .7 | 7 | 5 | 1 0 .9 | 1 4 .8 | No | 0 .2 | No | DE-BER | | 7 | 6 .1 | 1 8.2 | 0 .2 | 4 .2 | 8.4 | SE | 4 3 | SE | ESE | 1 9 | 2 6 | 6 3 | 4 7 | 1 0 2 4 .6 | 1 0 2 2 .2 | 4 | 6 | 1 2 .4 | 1 7 .3 | No | 0 | No | DE-BER | | 8 | 8.3 | 1 7 | 0 | 5 .6 | 4 .6 | E | 4 1 | SE | E | 1 1 | 2 4 | 6 5 | 5 7 | 1 0 2 6 .2 | 1 0 2 4 .2 | 6 | 7 | 1 2 .1 | 1 5 .5 | No | 0 | No | DE-BER | | 9 | 8.8 | 1 9.5 | 0 | 4 | 4 .1 | S | 4 8 | E | ENE | 1 9 | 1 7 | 7 0 | 4 8 | 1 0 2 6 .1 | 1 0 2 2 .7 | 7 | 7 | 1 4 .1 | 1 8.9 | No | 1 6 .2 | Yes | DE-BER | | 1 0 | 8.4 | 2 2 .8 | 1 6 .2 | 5 .4 | 7 .7 | E | 3 1 | S | ESE | 7 | 6 | 82 | 3 2 | 1 0 2 4 .1 | 1 0 2 0 .7 | 7 | 1 | 1 3 .3 | 2 1 .7 | Yes | 0 | No | DE-BER | | 1 1 | 9.1 | 2 5 .2 | 0 | 4 .2 | 1 1 .9 | N | 3 0 | SE | NW | 6 | 9 | 7 4 | 3 4 | 1 0 2 4 .4 | 1 0 2 1 .1 | 1 | 2 | 1 4 .6 | 2 4 | No | 0 .2 | No | DE-BER | | 1 2 | 8.5 | 2 7 .3 | 0 .2 | 7 .2 | 1 2 .5 | E | 4 1 | E | NW | 2 | 1 5 | 5 4 | 3 5 | 1 0 2 3 .8 | 1 0 1 9.9 | 0 | 3 | 1 6 .8 | 2 6 | No | 0 | No | DE-BER | | 1 3 | 1 0 .1 | 2 7 .9 | 0 | 7 .2 | 1 3 | WNW | 3 0 | S | NW | 6 | 7 | 6 2 | 2 9 | 1 0 2 2 | 1 0 1 7 .1 | 0 | 1 | 1 7 | 2 7 .1 | No | 0 | No | DE-BER | | 1 4 | 1 2 .1 | 3 0 .9 | 0 | 6 .2 | 1 2 .4 | NW | 4 4 | WNW | W | 7 | 2 0 | 6 7 | 2 0 | 1 0 1 7 .3 | 1 0 1 3 .1 | 1 | 4 | 1 9.7 | 3 0 .7 | No | 0 | No | DE-BER | | 1 5 | 1 0 .1 | 3 1 .2 | 0 | 8.8 | 1 3 .1 | NW | 4 1 | S | W | 6 | 2 0 | 4 5 | 1 6 | 1 0 1 8.2 | 1 0 1 3 .7 | 0 | 1 | 1 8.7 | 3 0 .4 | No | 0 | No | DE-BER | When I execute TLA1 2 1 7 Then Table import_summary contains: | id | created_date | status | rows_imported | rows_skipped | | 1 | 2 0 1 9-0 1 -0 1 | 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 | rain_tomorrow | city_code | | 1 | 8 | 2 4 .3 | 0 | 3 .4 | 6 .3 | NW | 3 0 | SW | NW | 6 | 2 0 | 6 8 | 2 9 | 1 0 1 9.7 | 1 0 1 5 | 7 | 7 | 1 4 .4 | 2 3 .6 | No | 3 .6 | Yes | DE-BER | | 2 | 1 4 | 2 6 .9 | 3 .6 | 4 .4 | 9.7 | ENE | 3 9 | E | W | 4 | 1 7 | 80 | 3 6 | 1 0 1 2 .4 | 1 0 0 8.4 | 5 | 3 | 1 7 .5 | 2 5 .7 | Yes | 3 .6 | Yes | DE-BER | | 3 | 1 3 .7 | 2 3 .4 | 3 .6 | 5 .8 | 3 .3 | NW | 85 | N | NNE | 6 | 6 | 82 | 6 9 | 1 0 0 9.5 | 1 0 0 7 .2 | 8 | 7 | 1 5 .4 | 2 0 .2 | Yes | 3 9.8 | Yes | DE-BER | | 4 | 1 3 .3 | 1 5 .5 | 3 9.8 | 7 .2 | 9.1 | NW | 5 4 | WNW | W | 3 0 | 2 4 | 6 2 | 5 6 | 1 0 0 5 .5 | 1 0 0 7 | 2 | 7 | 1 3 .5 | 1 4 .1 | Yes | 2 .8 | Yes | DE-BER | | 5 | 7 .6 | 1 6 .1 | 2 .8 | 5 .6 | 1 0 .6 | SSE | 5 0 | SSE | ESE | 2 0 | 2 8 | 6 8 | 4 9 | 1 0 1 8.3 | 1 0 1 8.5 | 7 | 7 | 1 1 .1 | 1 5 .4 | Yes | 0 | No | DE-BER | | 6 | 6 .2 | 1 6 .9 | 0 | 5 .8 | 8.2 | SE | 4 4 | SE | E | 2 0 | 2 4 | 7 0 | 5 7 | 1 0 2 3 .8 | 1 0 2 1 .7 | 7 | 5 | 1 0 .9 | 1 4 .8 | No | 0 .2 | No | DE-BER | | 7 | 6 .1 | 1 8.2 | 0 .2 | 4 .2 | 8.4 | SE | 4 3 | SE | ESE | 1 9 | 2 6 | 6 3 | 4 7 | 1 0 2 4 .6 | 1 0 2 2 .2 | 4 | 6 | 1 2 .4 | 1 7 .3 | No | 0 | No | DE-BER | | 8 | 8.3 | 1 7 | 0 | 5 .6 | 4 .6 | E | 4 1 | SE | E | 1 1 | 2 4 | 6 5 | 5 7 | 1 0 2 6 .2 | 1 0 2 4 .2 | 6 | 7 | 1 2 .1 | 1 5 .5 | No | 0 | No | DE-BER | | 9 | 8.8 | 1 9.5 | 0 | 4 | 4 .1 | S | 4 8 | E | ENE | 1 9 | 1 7 | 7 0 | 4 8 | 1 0 2 6 .1 | 1 0 2 2 .7 | 7 | 7 | 1 4 .1 | 1 8.9 | No | 1 6 .2 | Yes | DE-BER | | 1 0 | 8.4 | 2 2 .8 | 1 6 .2 | 5 .4 | 7 .7 | E | 3 1 | S | ESE | 7 | 6 | 82 | 3 2 | 1 0 2 4 .1 | 1 0 2 0 .7 | 7 | 1 | 1 3 .3 | 2 1 .7 | Yes | 0 | No | DE-BER | | 1 1 | 9.1 | 2 5 .2 | 0 | 4 .2 | 1 1 .9 | N | 3 0 | SE | NW | 6 | 9 | 7 4 | 3 4 | 1 0 2 4 .4 | 1 0 2 1 .1 | 1 | 2 | 1 4 .6 | 2 4 | No | 0 .2 | No | DE-BER | | 1 2 | 8.5 | 2 7 .3 | 0 .2 | 7 .2 | 1 2 .5 | E | 4 1 | E | NW | 2 | 1 5 | 5 4 | 3 5 | 1 0 2 3 .8 | 1 0 1 9.9 | 0 | 3 | 1 6 .8 | 2 6 | No | 0 | No | DE-BER | | 1 3 | 1 0 .1 | 2 7 .9 | 0 | 7 .2 | 1 3 | WNW | 3 0 | S | NW | 6 | 7 | 6 2 | 2 9 | 1 0 2 2 | 1 0 1 7 .1 | 0 | 1 | 1 7 | 2 7 .1 | No | 0 | No | DE-BER | | 1 4 | 1 2 .1 | 3 0 .9 | 0 | 6 .2 | 1 2 .4 | NW | 4 4 | WNW | W | 7 | 2 0 | 6 7 | 2 0 | 1 0 1 7 .3 | 1 0 1 3 .1 | 1 | 4 | 1 9.7 | 3 0 .7 | No | 0 | No | DE-BER | | 1 5 | 1 0 .1 | 3 1 .2 | 0 | 8.8 | 1 3 .1 | NW | 4 1 | S | W | 6 | 2 0 | 4 5 | 1 6 | 1 0 1 8.2 | 1 0 1 3 .7 | 0 | 1 | 1 8.7 | 3 0 .4 | No | 0 | No | DE-BER | | 1 6 | 1 2 .4 | 3 2 .1 | 0 | 8.4 | 1 1 .1 | E | 4 6 | SE | WSW | 7 | 9 | 7 0 | 2 2 | 1 0 1 7 .9 | 1 0 1 2 .8 | 0 | 3 | 1 9.1 | 3 0 .7 | No | 0 | No | DE-BER | | 1 7 | 1 3 .8 | 3 1 .2 | 0 | 7 .2 | 8.4 | ESE | 4 4 | WSW | W | 6 | 1 9 | 7 2 | 2 3 | 1 0 1 4 .4 | 1 0 0 9.8 | 7 | 6 | 2 0 .2 | 2 9.8 | No | 1 .2 | Yes | DE-BER | | 1 8 | 1 1 .7 | 3 0 | 1 .2 | 7 .2 | 1 0 .1 | S | 5 2 | SW | NE | 6 | 1 1 | 5 9 | 2 6 | 1 0 1 6 .4 | 1 0 1 3 | 1 | 5 | 2 0 .1 | 2 8.6 | Yes | 0 .6 | No | DE-BER | | 1 9 | 1 2 .4 | 3 2 .3 | 0 .6 | 7 .4 | 1 3 | E | 3 9 | NNE | W | 4 | 1 7 | 6 0 | 2 5 | 1 0 1 7 .1 | 1 0 1 3 .3 | 1 | 3 | 2 0 .2 | 3 1 .2 | No | 0 | No | DE-BER | | 2 0 | 1 5 .6 | 3 3 .4 | 0 | 8 | 1 0 .4 | NE | 3 3 | NNW | NNW | 2 | 1 3 | 6 1 | 2 7 | 1 0 1 8.5 | 1 0 1 3 .7 | 0 | 1 | 2 2 .8 | 3 2 | No | 0 | No | DE-BER | | 2 1 | 1 5 .3 | 3 3 .4 | 0 | 8.8 | 9.5 | WNW | 5 9 | N | NW | 2 | 3 1 | 6 0 | 2 6 | 1 0 1 2 .4 | 1 0 0 6 .5 | 1 | 5 | 2 2 .2 | 3 2 .8 | No | 0 .4 | No | DE-BER | | 2 2 | 1 6 .4 | 1 9.4 | 0 .4 | 9.2 | 0 | E | 2 6 | ENE | E | 6 | 1 1 | 88 | 7 2 | 1 0 1 0 .7 | 1 0 0 8.9 | 8 | 8 | 1 6 .5 | 1 8.3 | No | 2 5 .8 | Yes | DE-BER | Then Table import_log contains: | id | created_date | type | message | | 1 | 2 0 1 9-0 1 -0 1 | warning | Invalid city code "FR-PAR" | | 2 | 2 0 1 9-0 1 -0 1 | warning | Invalid city code "FR-PAR" | | 3 | 2 0 1 9-0 1 -0 1 | warning | Invalid city code "FR-PAR" | | 4 | 2 0 1 9-0 1 -0 1 | warning | Invalid city code "FR-PAR" | | 5 | 2 0 1 9-0 1 -0 1 | warning | Invalid city code "FR-PAR" | Many columns Many rows
35. I Stand With Ukraine • 46 new tests. • All scenarios covered. • No redundancies. • Easy to read and to maintain.
36. I Stand With Ukraine Scenario: Add warning for invalid city Given city exists for PL-POZ And file contains entry for PL-POZ / 2024-01-01 And file contains entry for CA-MTL / 2024-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
37. I Stand With Ukraine Existing Tests • Will the tests survive refactoring? • Effort of fixing vs rewriting tests. • Do the tests give you confidence? • Can you maintain these tests?
38. Questions? PHPERS SUMMIT 2024 | POZNAŃ, POLAND @afilina@phpc.social | @afilina