Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: refactor asserts to pytest style #1191

Merged
merged 13 commits into from
Apr 16, 2024

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Apr 15, 2024

Refactor unittest self.assert to pytest assert style.

I used a modified version of pytestify, so that it only converts the assertions without changing anything else. This will be phase 1 of our unittest-pytest conversion.

Commits Explained

The first commit is 100% generated by pytestify, there is no need to review it.

After the first commit, two test files weren't processed because of bugs in pytestify, which I couldn't be bothered to fix, so I did 3 separate commits to hack the test cases to make pytestify work and revert the hacks:

These three continuous commits are only meant to make pytestify work on certain test files.

At this point, the UT still fails.

The fifth commit is where the major manual work is done and made the test pass. Please spend most of your time reviewing this commit since it's where the manual work is. Major changes include:

  • fix multiline strings pytestify failed to handle
  • update self.assertRaises and with self.assertRaisesRegex which pytestify couldn't handle
  • fix self.assert within lambda functions
  • fix pytestify bugs on test cases defined in a list
  • add missing imports
  • fix real pebble tests
  • fix linting errors

There might be more types of changes that I have forgotten because it took quite some time to work on every single test file.

The sixth commit is a minor change where an exception is added to pyproject.toml to make one UT pass.

Before/After UT Result Comparison

TL;DR: According to UT statistics, the number of test cases is the same after the refactor, there isn't any test case accidentally left behind.

Before refactoring:

$ python -m tox -e unit
unit: install_deps> python -I -m pip install 'coverage[toml]~=7.0' 'pytest~=7.2' 'PyYAML==6.*' 'typing_extensions~=4.2' 'websocket-client==1.*'
unit: commands[0]> coverage run --source=ops/ -m pytest --ignore=test/smoke -v --tb native
=================================================================================================================================================== test session starts ====================================================================================================================================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0 -- /Users/tiexin/work/operator2/.tox/unit/bin/python
cachedir: .tox/unit/.pytest_cache
rootdir: /Users/tiexin/work/operator2
collected 961 items

...

================================================================================================================================== 925 passed, 36 skipped, 1 warning in 340.29s (0:05:40) ==================================================================================================================================
unit: commands[1]> coverage report
Name                       Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------
ops/__init__.py               10      0      0      0   100%
ops/_private/__init__.py       0      0      0      0   100%
ops/_private/timeconv.py      61      0     28      0   100%
ops/_private/yaml.py           9      0      0      0   100%
ops/charm.py                 643     28    177     17    94%   50-80, 270, 288, 420, 453, 458-460, 474->476, 488, 497-498, 603, 655->659, 670->exit, 672->exit, 676, 681, 744->746, 757, 869, 953, 1164-1165, 1342, 1607-1608, 1774
ops/framework.py             685     54    270     37    89%   62->exit, 64->exit, 65->exit, 66->exit, 123, 126, 164->169, 166->169, 170, 280, 318, 391-392, 534, 559, 562, 597-598, 671->675, 722, 746, 780, 795, 905-907, 927->948, 1062-1066, 1085, 1089, 1094, 1153, 1160, 1181, 1210, 1226, 1257, 1260, 1264, 1268, 1306, 1310, 1314, 1318, 1322, 1326, 1330, 1334, 1338, 1342, 1392, 1396, 1399-1404, 1408, 1412
ops/jujuversion.py            80      6     48      6    91%   53, 57, 62, 66, 76, 80
ops/lib/__init__.py          176     25     77      8    85%   114, 129-131, 139-141, 146-147, 149-150, 155-156, 173-179, 189-193, 219->207, 261->265
ops/log.py                    27      4      4      0    81%   67-73
ops/main.py                  262    117     92      9    49%   56-62, 75-100, 118-122, 132-143, 147, 152-220, 264, 281-301, 307, 331-332, 375-376, 408->410, 455->460, 465-469, 484, 504->509, 546
ops/model.py                1523     77    624     30    94%   301, 313->exit, 315->exit, 429, 479, 784, 800, 832, 915->918, 922, 925, 928, 1035, 1172, 1181-1186, 1269, 1519, 1556, 1617, 1637, 1669, 1781, 1899->1901, 2050, 2053, 2083->2086, 2149->2151, 2239, 2243, 2487, 2544, 2567, 2579, 2589, 2657, 2679, 2761, 2810, 2833, 2850, 2856, 2992, 3016, 3028, 3078, 3127-3128, 3152-3153, 3157-3170, 3225-3235, 3239, 3351, 3382-3385, 3393->3395, 3436->3439, 3481
ops/pebble.py               1279     66    431     36    94%   172->exit, 184->exit, 186->exit, 190->exit, 192->exit, 194->exit, 204-267, 283->exit, 285->exit, 287->exit, 289->exit, 291->exit, 310, 319, 322->exit, 359, 392, 419, 452, 536, 572, 605, 616, 661, 678, 726, 862, 949, 1011, 1066, 1121, 1222, 1294-1295, 1305, 1518->1520, 1559, 1602, 1653, 1740, 1741->1743, 1806, 1809, 1820-1823, 1830, 2049->2052, 2061, 2130, 2134, 2183, 2185, 2201, 2430, 2452, 2712-2717, 2720-2722, 2865->2869, 2891, 2898, 2901, 2911, 2915->exit, 2924-2925
ops/storage.py               159      1     30      1    99%   306
ops/testing.py              1580    127    764     69    89%   508->510, 533->535, 562->564, 638->637, 870->872, 885->888, 914->922, 923->927, 936, 953, 986, 992, 1044, 1059->exit, 1073, 1192->exit, 1243, 1247->1252, 1264->1262, 1329->1327, 1635, 1638->1640, 1665, 1670, 1758-1760, 1908, 1916, 2038, 2123, 2147, 2152-2158, 2161, 2256-2257, 2298, 2313-2332, 2353->2358, 2402, 2405, 2459, 2518-2520, 2528, 2532, 2535, 2569, 2581, 2591->exit, 2593, 2617, 2638->2637, 2668-2671, 2692, 2699-2702, 2704-2707, 2709, 2711-2714, 2770, 2781, 2786, 2846, 2862, 2868, 2876, 2916, 2919, 2925, 2928, 2931, 2947, 2994-3008, 3013, 3021, 3028, 3048->3037, 3053, 3249-3250, 3329-3330, 3340, 3386, 3406-3430, 3433, 3449-3450, 3509, 3523, 3525-3527, 3530, 3547, 3549
ops/version.py                 2      0      0      0   100%
----------------------------------------------------------------------
TOTAL                       6496    505   2545    213    90%
  unit: OK (345.16=setup[2.65]+cmd[341.91,0.60] seconds)
  congratulations :) (345.19 seconds)

After refactoring:

$ python -m tox -e unit
unit: install_deps> python -I -m pip install 'coverage[toml]~=7.0' 'pytest~=7.2' 'PyYAML==6.*' 'typing_extensions~=4.2' 'websocket-client==1.*'
unit: commands[0]> coverage run --source=ops/ -m pytest --ignore=test/smoke -v --tb native
=================================================================================================================================================== test session starts ====================================================================================================================================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0 -- /Users/tiexin/work/operator/.tox/unit/bin/python
cachedir: .tox/unit/.pytest_cache
rootdir: /Users/tiexin/work/operator
collected 961 items

...

================================================================================================================================== 925 passed, 36 skipped, 1 warning in 352.08s (0:05:52) ==================================================================================================================================
unit: commands[1]> coverage report
Name                       Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------
ops/__init__.py               10      0      0      0   100%
ops/_private/__init__.py       0      0      0      0   100%
ops/_private/timeconv.py      61      0     28      0   100%
ops/_private/yaml.py           9      0      0      0   100%
ops/charm.py                 643     28    177     17    94%   50-80, 270, 288, 420, 453, 458-460, 474->476, 488, 497-498, 603, 655->659, 670->exit, 672->exit, 676, 681, 744->746, 757, 869, 953, 1164-1165, 1342, 1607-1608, 1774
ops/framework.py             685     54    270     37    89%   62->exit, 64->exit, 65->exit, 66->exit, 123, 126, 164->169, 166->169, 170, 280, 318, 391-392, 534, 559, 562, 597-598, 671->675, 722, 746, 780, 795, 905-907, 927->948, 1062-1066, 1085, 1089, 1094, 1153, 1160, 1181, 1210, 1226, 1257, 1260, 1264, 1268, 1306, 1310, 1314, 1318, 1322, 1326, 1330, 1334, 1338, 1342, 1392, 1396, 1399-1404, 1408, 1412
ops/jujuversion.py            80      6     48      6    91%   53, 57, 62, 66, 76, 80
ops/lib/__init__.py          176     25     77      8    85%   114, 129-131, 139-141, 146-147, 149-150, 155-156, 173-179, 189-193, 219->207, 261->265
ops/log.py                    27      4      4      0    81%   67-73
ops/main.py                  262    117     92      9    49%   56-62, 75-100, 118-122, 132-143, 147, 152-220, 264, 281-301, 307, 331-332, 375-376, 408->410, 455->460, 465-469, 484, 504->509, 546
ops/model.py                1523     77    624     30    94%   301, 313->exit, 315->exit, 429, 479, 784, 800, 832, 915->918, 922, 925, 928, 1035, 1172, 1181-1186, 1269, 1519, 1556, 1617, 1637, 1669, 1781, 1899->1901, 2050, 2053, 2083->2086, 2149->2151, 2239, 2243, 2487, 2544, 2567, 2579, 2589, 2657, 2679, 2761, 2810, 2833, 2850, 2856, 2992, 3016, 3028, 3078, 3127-3128, 3152-3153, 3157-3170, 3225-3235, 3239, 3351, 3382-3385, 3393->3395, 3436->3439, 3481
ops/pebble.py               1279     66    431     36    94%   172->exit, 184->exit, 186->exit, 190->exit, 192->exit, 194->exit, 204-267, 283->exit, 285->exit, 287->exit, 289->exit, 291->exit, 310, 319, 322->exit, 359, 392, 419, 452, 536, 572, 605, 616, 661, 678, 726, 862, 949, 1011, 1066, 1121, 1222, 1294-1295, 1305, 1518->1520, 1559, 1602, 1653, 1740, 1741->1743, 1806, 1809, 1820-1823, 1830, 2049->2052, 2061, 2130, 2134, 2183, 2185, 2201, 2430, 2452, 2712-2717, 2720-2722, 2865->2869, 2891, 2898, 2901, 2911, 2915->exit, 2924-2925
ops/storage.py               159      1     30      1    99%   306
ops/testing.py              1580    127    764     69    89%   508->510, 533->535, 562->564, 638->637, 870->872, 885->888, 914->922, 923->927, 936, 953, 986, 992, 1044, 1059->exit, 1073, 1192->exit, 1243, 1247->1252, 1264->1262, 1329->1327, 1635, 1638->1640, 1665, 1670, 1758-1760, 1908, 1916, 2038, 2123, 2147, 2152-2158, 2161, 2256-2257, 2298, 2313-2332, 2353->2358, 2402, 2405, 2459, 2518-2520, 2528, 2532, 2535, 2569, 2581, 2591->exit, 2593, 2617, 2638->2637, 2668-2671, 2692, 2699-2702, 2704-2707, 2709, 2711-2714, 2770, 2781, 2786, 2846, 2862, 2868, 2876, 2916, 2919, 2925, 2928, 2931, 2947, 2994-3008, 3013, 3021, 3028, 3048->3037, 3053, 3249-3250, 3329-3330, 3340, 3386, 3406-3430, 3433, 3449-3450, 3509, 3523, 3525-3527, 3530, 3547, 3549
ops/version.py                 2      0      0      0   100%
----------------------------------------------------------------------
TOTAL                       6496    505   2545    213    90%
  unit: OK (357.19=setup[3.19]+cmd[353.40,0.61] seconds)
  congratulations :) (357.22 seconds)

Note that at the moment this branch is out of date with the main branch because after checking out this branch new changes including tests are made to the main branch. Since I suppose the reviewing of this PR could take some time, I will not try to sync with the main branch multiple times to work out the conflicts. I will take a final merge and conflict-solving after review, I hope this is OK.

@IronCore864 IronCore864 marked this pull request as ready for review April 15, 2024 06:55
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good, thanks! It's a pity pytestify doesn't do more for us, like assertRaises. We could try to fix it ourselves, but I don't think it's worth it. I think the new asserts are much easier to read. And thanks for doing the coverage diff -- looks good!

Just one comment about the assert statement in lambdas: I'd like to improve the error messages in those cases if possible.

Also, if you've modified pytestify, do you have a link to the diff for the modified version (on your own fork)?

test/test_framework.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to review the 7 large files - will come back to that later today.

test/test_private.py Show resolved Hide resolved
test/test_private.py Outdated Show resolved Hide resolved
test/test_real_pebble.py Outdated Show resolved Hide resolved
test/test_storage.py Outdated Show resolved Hide resolved
test/test_storage.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM :)

test/test_testing.py Outdated Show resolved Hide resolved
test/test_private.py Show resolved Hide resolved
test/test_private.py Show resolved Hide resolved
@IronCore864 IronCore864 merged commit ab239e1 into canonical:main Apr 16, 2024
27 checks passed
@IronCore864 IronCore864 deleted the pytest-phase1 branch April 16, 2024 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants