Adding Tests to Untestable Legacy Code PHPERS SUMMIT 2024 | POZNAŃ, POLAND @afilina@phpc.social | @afilina

I Stand With Ukraine “We need tests to refactor, but we need to refactor to make the code testable.”

I Stand With Ukraine Anna Filina • Coding since 1997. • PHP, Java, C#, Flash, etc. • Legacy archaeology. • Test automation. • Mentoring. • Filina Consulting.

I Stand With Ukraine

I Stand With Ukraine No Tests

I Stand With Ukraine

I Stand With Ukraine Write tests as part of other work.

I Stand With Ukraine • Bug report. • New feature. • Refactoring. • Build coverage over time.

I Stand With Ukraine Not Unit-Testable

I Stand With Ukraine • Might bootstrap the framework. • Hit the database. • Call external APIs. • Require extensive mocking.

I Stand With Ukraine Unit tests end up being convoluted and not useful.

I Stand With Ukraine

I Stand With Ukraine

I Stand With Ukraine

I Stand With Ukraine • Break up class. • Extract method. • Change method signature. • Reorganize dependencies.

I Stand With Ukraine

I Stand With Ukraine

I Stand With Ukraine ASP Classic

I Stand With Ukraine PHP

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

I Stand With Ukraine • Behat • Codeception

I Stand With Ukraine Slower but fewer

I Stand With Ukraine Unit Tests

I Stand With Ukraine

I Stand With Ukraine

I Stand With Ukraine class ProductController extends AbstractController { public function search() { //... $products = $this->repository->search($criteria); return $this->render("products/search.html.twig", $products); } }

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); } }

I Stand With Ukraine New Tests • Write characterization that survive refactoring. • Write unit tests for new or refactored code.

I Stand With Ukraine Broken Tests

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.

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.

I Stand With Ukraine Afraid to throw code away

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

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

I Stand With Ukraine • 46 new tests. • All scenarios covered. • No redundancies. • Easy to read and to maintain.

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

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?

Questions? PHPERS SUMMIT 2024 | POZNAŃ, POLAND @afilina@phpc.social | @afilina