Improve Unit Test Experience for Functionality using Azure CLI Configuration

[PramodValavala-MSFT]

Is your feature request related to a problem? Please describe.

When writing unit test for functionality that leverages Azure CLI Configuration, the config is persisted across tests leading to misleading results.

Most recently, new functionality in the az bicep sub-command leveraged Azure CLI Configuration but a bug slipped through the tests due to the config value not being reset with each test, causing numerous build pipeline failures for customers.

Describe the solution you'd like

Better unit testing experience for such functionality. Based on this last experience, here are some thoughts I had:

• Reset config for each test or In-Memory Config for DummyCLI
• Should Config API throw error for No Section
• Lint Rule for cli_ctx.config.get() without default value. Warning at the minimum.

[yonzhan]

ZelinWang (@wangzelin007) for awareness

[wangzelin007]

Hi Pramod Valavala (@PramodValavala-MSFT),

• Reset config for each test or In-Memory Config for DummyCLI
  Our test framework already supports setUp and tearDown for each test
• Should Config API throw error for No Section
  Jiashuo Li (@jiasli) for awareness
• Lint Rule for cli_ctx.config.get() without default value. Warning at the minimum.
  Jiashuo Li (@jiasli) for awareness

[PramodValavala-MSFT]

ZelinWang (@wangzelin007) Thanks for the update!

While the setUp and tearDown methods are supported, there is still no way to clear the config I believe. Should we just remove the config file manually?

I suppose this functionality is more in the knack framework than Azure CLI, perhaps this should be implemented there?

[jiasli]

Sorry but I didn't really get the questions here. The behavior of cli_ctx.config.get() is internally implemented by the configparser built-in library.

• Should Config API throw error for No Section

Is this a question or conditional?

The current behavior of cli_ctx.config.get('aaa', 'bbb') is to raise configparser.NoSectionError:

  File "D:\cli\py310\lib\site-packages\knack\config.py", line 208, in get
    return self.config_parser.get(section, option)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python310\lib\configparser.py", line 783, in get
    d = self._unify_values(section, vars)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python310\lib\configparser.py", line 1154, in _unify_values
    raise NoSectionError(section) from None
configparser.NoSectionError: No section: 'aaa'

• Lint Rule for cli_ctx.config.get() without default value. Warning at the minimum.

Without specifying fallback kwarg, cli_ctx.config.get('core', 'bbb') errs:

  File "D:\cli\py310\lib\site-packages\knack\config.py", line 208, in get
    return self.config_parser.get(section, option)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python310\lib\configparser.py", line 794, in get
    raise NoOptionError(option, section)
configparser.NoOptionError: No option 'bbb' in section: 'core'

It can't simply warns since it doesn't know which value to use.

[bebound]

After this PR is merged, each test can use random config dir.
#25689

[PramodValavala-MSFT]

Jiashuo Li (@jiasli) Thanks for the insights! This behavior is difficult to catch in tests, which led to the issue here, because the config file is re-used across tests, and in our case, we never got the NoSectionError.

Since this is the expected behavior and I assume authors either must ensure a fallback value is present or handle the NoSectionError exception, I was thinking it would be good to have a linter rule to highlight this as a warning that this line might throw this exception without the fallback value provided.

Since this was the first time I was using this API, it wasn't obvious to me that this line could raise an exception and testing it missed as well since the config file was being shared.

Hang (@bebound) Yes. I think that should help. So, will we be able to change the config dir for each test as part of the setUp method or would it be automatic as part of the test framework?

Also, considering there is a performance impact, wouldn't it make sense to make the config file in-memory and have an API to clear it before each test to speed things up?