My idea is to make the parser self-contained as much as possible -- its properties shouldn't be changed from outside the parser except for test hooks.

Specifically, ConfigurationName should be properly set within the parser, so after calling _parser.Parse(args), _parser should have the correct ConfigurationName value without needing to do the following:

_parser.ConfigurationName = CommandLineParameterParser.GetConfigurationNameFromGroupPolicy();

ConsoleHost is owner of the parser (s_cpp), it defines how use results of command line parsing so I believe all logic should be in ConsoleHost, not in the parser.
It would be surprising if the PowerShell parser started compiling or interpreting or making WMI queries.

I don't think it really matters to make CommandLineParser a pure argument parser that doesn't do anything else, and I can imagine that future changes to it for additional flags may likely add simple side effect operations to it. I don't see that's a problem, as long as the new flags are testable with test-hook properties.

I don't see that's a problem

Oh, I spent many time to debug new tests because the parser has side effects. If you catch the issue again it confirms that the design is not so good and the worst thing is that this again provokes the addition of workarounds not only in tests but also in the main code. This is what I tried to avoid in my PR. Sorry for long discussion.

I spent many time to debug new tests because the parser has side effects.

It reveals the fact that the code was not written in a testable way, and hence I added the constructor for testing purpose and the _isForTesting field.

Again, I personally don't care that much to make the argument parser a pure parser without side effect, and it's likely we cannot hold that true in future changes, unless you can catch and review all subsequent changes to the argument parser :).

For now, I care more about fixing the test failure in debug build, so I will just go with your approach for this PR :)

It reveals the fact that the code was not written in a testable way, and hence I added the constructor for testing purpose and the _isForTesting field.

Again, I personally don't care that much to make the argument parser a pure parser without side effect, and it's likely we cannot hold that true in future changes, unless you can catch and review all subsequent changes to the argument parser :).

To express a personal preference, I think:

• Testability and purity are closely related, and both also help code quality and robustness
• Using flags to change side-effects into testable state means testing code paths and production code paths aren't the same
• My ideal would be for the argument parser to process the string[] args input and spit out an object that we could either test or send to the consuming ConsoleHost component. Similar to this .NET library or the prevailing Rust library. (System.CommandLine operates at a different level, so doesn't really compare here)

With that said, I know we ultimately want to minimise code churn and maximise performance, so an "ideal" design may not be the right move here

I know we ultimately want to minimise code churn and maximise performance

As side value, after decoupling the parser we can easy measure its performances with dotnet test.

As side thought, we do xUnit tests based exclusively on SMA and we could test decoupling (independent from SMA) code in separate xUnit tests to check the algorithms used.

The Windows test fails

[-] Pre-Requisistes link for 'WMF 4.0' is reachable: https://www.microsoft.com/download/details.aspx?id=40855

The link does not work https://www.microsoft.com/download/details.aspx?id=40855

I think it is important for Windows Installer.

/cc @TravisEz13 @adityapatwardhan @SteveL-MSFT

Error 404 - Not Found

The Windows test fails

We're working on this. Unclear why it's failing

That test was disabled by #13479. The failure was tracked by #13478

@rjmholt I think this one is ready to merge.

🎉v7.1.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes