bpo-30635: Fix refleak in test_c_locale_coercion#2126
bpo-30635: Fix refleak in test_c_locale_coercion#2126ncoghlan merged 4 commits intopython:masterfrom vstinner:test_c_locale_coercion
Conversation
When checking for reference leaks, test_c_locale_coercion is run multiple times and so _LocaleCoercionTargetsTestCase.setUpClass() is called multiple times. setUpClass() appends new value at each call. Use _LocaleCoercionTargetsTestCase.available_targets as a marker to check if the class is already initialized or not.
Convert setUpClass() to setUpModule() to only compute AVAILABLE_TARGETS once to spawn less processes. Speed up test_c_locale_coercion: 3.2 sec => 2.9 sec.
matrixise
left a comment
matrixise
left a comment
There was a problem hiding this comment.
this PR fixes the issue with the leak.
ncoghlan
left a comment
ncoghlan
left a comment
There was a problem hiding this comment.
Thanks for this, but it's currently a bit too aggressive in skipping test cases.
Moving the skip logic back into the class setup, but leaving the calculation of AVAILABLE_TARGETS at module level should do the trick on that front.
| if _set_locale_in_subprocess(target_locale): | ||
| AVAILABLE_TARGETS.append(target_locale) | ||
| if not AVAILABLE_TARGETS: | ||
| raise unittest.SkipTest("No C-with-UTF-8 locale available") |
There was a problem hiding this comment.
There are other tests in the module that should be run regardless of whether or not these locales are available.
There was a problem hiding this comment.
Ah you merged my PR even if I didn't address your comment: "There are other tests in the module that should be run regardless of whether or not these locales are available." ?
There was a problem hiding this comment.
Yeah, I realised that since I was updating this test case anyway, I could just incorporate that into #2130
|
|
||
| class _LocaleCoercionTargetsTestCase(_ChildProcessEncodingTestCase): | ||
| # Base class for test cases that rely on coercion targets being defined | ||
| pass |
There was a problem hiding this comment.
The cls.targets_required and not AVAILABLE_TARGETS check should still be handled in setupClass so that the other tests still run as expected, even when locale coercion isn't possible.
| expected_warning.append(self.EXPECTED_COERCION_WARNING) | ||
| # Expect coercion to use the first available locale | ||
| warning_msg = CLI_COERCION_WARNING_FMT.format(AVAILABLE_TARGETS[0]) | ||
| expected_warning.append(warning_msg) |
There was a problem hiding this comment.
Good catch, I'd missed that that particular simplification was possible now.
|
I need to make other updates to this test case anyway, so I can handle moving the skip back into |
4 participants
Fix a reference leak in test_c_locale_coercion and refactor the code to make it faster.