Skip to content

bpo-30635: Fix refleak in test_c_locale_coercion #2126

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.

Sign up for GitHub

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

Merged
ncoghlan merged 4 commits into from
Jun 13, 2017
Merged

bpo-30635: Fix refleak in test_c_locale_coercion #2126

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7173afc
bpo-30635: Fix refleak in test_c_locale_coercion
vstinner Jun 12, 2017
40a8f4b
test_c_locale_coercion: remove unused targets_required
vstinner Jun 12, 2017
7ab9253
test_c_locale_coercion: inline EXPECTED_COERCION_WARNING
vstinner Jun 12, 2017
ec74486
test_c_locale_coercion: only create AVAILABLE_TARGETS once
vstinner Jun 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 25 additions & 21 deletions Lib/test/test_c_locale_coercion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,27 +143,29 @@ def test_library_c_locale_warning(self):
"or PYTHONCOERCECLOCALE=0 to disable this locale coercion behavior)."
)

class _LocaleCoercionTargetsTestCase(_ChildProcessEncodingTestCase):
# Base class for test cases that rely on coercion targets being defined

available_targets = []
targets_required = True
AVAILABLE_TARGETS = None

@classmethod
def setUpClass(cls):
first_target_locale = None
available_targets = cls.available_targets
# Find the target locales available in the current system
for target_locale in _C_UTF8_LOCALES:
if _set_locale_in_subprocess(target_locale):
available_targets.append(target_locale)
if first_target_locale is None:
first_target_locale = target_locale
if cls.targets_required and not available_targets:
raise unittest.SkipTest("No C-with-UTF-8 locale available")
# Expect coercion to use the first available locale
warning_msg = CLI_COERCION_WARNING_FMT.format(first_target_locale)
cls.EXPECTED_COERCION_WARNING = warning_msg
def setUpModule():
global AVAILABLE_TARGETS

if AVAILABLE_TARGETS is not None:
# initialization already done
return
AVAILABLE_TARGETS = []

# Find the target locales available in the current system
for target_locale in _C_UTF8_LOCALES:
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")
Copy link
Contributor

@ncoghlan ncoghlan Jun 12, 2017

Choose a reason for hiding this comment

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

There are other tests in the module that should be run regardless of whether or not these locales are available.

Copy link
Member Author

@vstinner vstinner Jun 13, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ncoghlan ncoghlan Jun 13, 2017

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@ncoghlan ncoghlan Jun 12, 2017

Choose a reason for hiding this comment

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

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.



class LocaleConfigurationTests(_LocaleCoercionTargetsTestCase):
Expand All @@ -183,7 +185,7 @@ def test_external_target_locale_configuration(self):
"LC_ALL": "",
}
for env_var in ("LANG", "LC_CTYPE"):
for locale_to_set in self.available_targets:
for locale_to_set in AVAILABLE_TARGETS:
with self.subTest(env_var=env_var,
configured_locale=locale_to_set):
var_dict = base_var_dict.copy()
Expand Down Expand Up @@ -215,7 +217,9 @@ def _check_c_locale_coercion(self, expected_fsencoding, coerce_c_locale):

expected_warning = []
if coerce_c_locale != "0":
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)
Copy link
Contributor

@ncoghlan ncoghlan Jun 12, 2017

Choose a reason for hiding this comment

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

Good catch, I'd missed that that particular simplification was possible now.


base_var_dict = {
"LANG": "",
Expand Down