Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Adding Tests to Untestable Legacy Code

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.

Anna Filina

October 05, 2021
Tweet

More Decks by Anna Filina

Other Decks in Programming

Transcript

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

    View full-size 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 full-size 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 full-size slide

  4. No Tests
    Hard to justify a
    testing spree.

    View full-size slide

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

    View full-size slide

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

    View full-size slide

  7. Not Unit-Testable

    View full-size 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 full-size slide

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

    View full-size slide

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

    View full-size slide

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

    View full-size 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 full-size slide

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

    View full-size slide

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

    View full-size slide

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

    View full-size slide

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

    View full-size 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 full-size slide

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

    View full-size slide

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

    View full-size slide

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

    View full-size slide

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

    View full-size slide

  22. 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 full-size slide

  23. 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 full-size slide

  24. • 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 full-size slide

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

    View full-size slide

  26. • 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 full-size slide

  27. • 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 full-size slide

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

    View full-size slide

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

    View full-size slide

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

    View full-size slide

  31. 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 full-size slide

  32. 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 full-size slide

  33. • 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 full-size slide

  34. Thank you!
    @afilina

    View full-size slide