{testsdk} Initialize random config dir in setUp
#28849
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌AzureCLI-FullTest
|
azure-client-tools-bot-prd
bot
added this to the May 2024 (2024-05-21)❗ Breaking change release ❗ milestone
️✔️AzureCLI-BreakingChangeTest
|
|
testsdk |
jiasli
commented
Apr 28, 2024
| # 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.
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
jiasli
commented
Apr 28, 2024
|
|
||
| # 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.
GLOBAL_CONFIG_DIR will always exist.
There was a problem hiding this comment.
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
The
super().__init__() is not called yet when this line is executed, and the folder might not exist.
jiasli
commented
Apr 28, 2024
| def tearDown(self): | ||
| for processor in self._processors_to_reset: | ||
| processor.reset() | ||
| if self.random_config_dir: |
There was a problem hiding this comment.
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.
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.
cli_ctx is moved to setUp mothod, which is executed after __init__.
If cli_ctx is used in __init__, this error is raised.
jiasli
modified the milestones:
May 2024 (2024-05-21)❗ Breaking change release ❗,
June 2024 (2024-07-02)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #28848
Description
According to https://docs.python.org/3/library/unittest.html#unittest.TestCase.tearDown,
tearDownis a counterpart ofsetUp, meaning anything that is done intearDownshould have its equivalent insetUp.This PR solves 2 problems:
config_dirleak: By injectingprint(os.getpid(), method_name)intoazure.cli.testsdk.base.ScenarioTest.__init__and runningazdev testin--seriesmode , we can see pytest first creates aunittest.TestCasewithmethod_name="runTest", and runs no test with it:As no test method is run for
runTest,tearDownis not called, leading to random config dir not being deleted.patch.dictpollution: From the above log, we can seeazure.cli.testsdk.base.ScenarioTest.__init__is run at pytest's collecting staging, in the same process (1098), buttearDownis 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:
--liveandrandom_config_diris enabled #26475random_config_dirwhenAZURE_CONFIG_DIRis set #28673