-
Notifications
You must be signed in to change notification settings - Fork 3k
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
{testsdk} Initialize random config dir in setUp
#28849
base: dev
Are you sure you want to change the base?
Conversation
❌AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
testsdk |
# get_config_dir directly. We need to set the env to make sure the config_dir is used. | ||
env_patch = patch.dict(os.environ, {'AZURE_CONFIG_DIR': config_dir}) | ||
env_patch.start() | ||
self.addCleanup(env_patch.stop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The official doc of unittest.mock
also prefers addCleanup
over tearDown
: https://docs.python.org/3/library/unittest.mock.html#patch-methods-start-and-stop
|
||
# In recording mode, copy login credentials from global config dir to the dummy config dir | ||
if os.getenv(ENV_VAR_TEST_LIVE, '').lower() == 'true': | ||
if os.path.exists(GLOBAL_CONFIG_DIR): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GLOBAL_CONFIG_DIR
will always exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, if the user is using --live
, .azure
should exist.
But from the code, .azure
is created by AzCli
:
azure-cli/src/azure-cli-core/azure/cli/core/__init__.py
Lines 77 to 78 in f98ba80
azure_folder = self.config.config_dir | |
ensure_dir(azure_folder) |
The
super().__init__()
is not called yet when this line is executed, and the folder might not exist.
def tearDown(self): | ||
for processor in self._processors_to_reset: | ||
processor.reset() | ||
if self.random_config_dir: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if
block is also eliminated by using addCleanup
.
@@ -83,7 +83,6 @@ class ScenarioTest(ReplayableTest, CheckerMixin, unittest.TestCase): | |||
def __init__(self, method_name, config_file=None, recording_name=None, | |||
recording_processors=None, replay_processors=None, recording_patches=None, replay_patches=None, | |||
random_config_dir=False): | |||
self.cli_ctx = get_dummy_cli(random_config_dir=random_config_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests reported the following errors:
AttributeError: 'xxxScenarioTest' object has no attribute 'cli_ctx'
https://dev.azure.com/azclitools/public/_build/results?buildId=153167&view=logs&j=9ccb9437-6abb-5c1a-b909-dd7aa8d5d916&t=da342424-93bf-5216-b53c-16ba29f2062e&l=674
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cli_ctx
is moved to setUp
mothod, which is executed after __init__
.
If cli_ctx
is used in __init__
, this error is raised.
Fix #28848
Description
According to https://docs.python.org/3/library/unittest.html#unittest.TestCase.tearDown,
tearDown
is a counterpart ofsetUp
, meaning anything that is done intearDown
should have its equivalent insetUp
.This PR solves 2 problems:
config_dir
leak: By injectingprint(os.getpid(), method_name)
intoazure.cli.testsdk.base.ScenarioTest.__init__
and runningazdev test
in--series
mode , we can see pytest first creates aunittest.TestCase
withmethod_name="runTest"
, and runs no test with it:As no test method is run for
runTest
,tearDown
is not called, leading to random config dir not being deleted.patch.dict
pollution: From the above log, we can seeazure.cli.testsdk.base.ScenarioTest.__init__
is run at pytest's collecting staging, in the same process (1098
), buttearDown
is executed after test methods have been finished. Therefore, thispatch.dict(os.environ, {'AZURE_CONFIG_DIR': config_dir})
statement pollutes other test classes'__init__
methods while pytest collects them.Related:
--live
andrandom_config_dir
is enabled #26475random_config_dir
whenAZURE_CONFIG_DIR
is set #28673