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
   HEISENBUG MOSCOW | OCT 5, 2021 @afilina
   

   View Slide

2. "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.
   

   View Slide

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

   View Slide

4. No Tests
   Hard to justify a
   testing spree.
   

   View Slide

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

   View Slide

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

   View Slide

7. Not Unit-Testable
   

   View Slide

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

   View Slide

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

   View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

17. 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.
    

    View Slide

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

    View Slide

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

    View Slide

20. Unit Tests
    

    View Slide

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

    View Slide

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

    View Slide

23. 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.
    

    View Slide

24. 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.
    

    View Slide

25. • 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.
    

    View Slide

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

    View Slide

27. • 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.
    

    View Slide

28. • 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.
    

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

32. 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.
    

    View Slide

33. 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.
    

    View Slide

34. • 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.
    

    View Slide

35. Thank you!
    @afilina
    

    View Slide