Add unit tests for ArgParser and improve output of --help

[YoshiRulz]

From 5c73d5b#r177008277.

If this is merged, notify dotnet/command-line-api issue 2765.

[SuuperW]

The arg parser code would benefit from better documentation. Even the code before your changes has some parts that don't make any sense, like sending all the args to stderr. Even what does make sense after careful reading would still benefit from some comments. My being unfamiliar with System.CommandLine makes it worse.

[SuuperW]

Why did you add this parameter? None of your various uses are explained. The one making it always return 0 seems to make the unit test pointless.

[YoshiRulz]

I want to avoid writing to the console during unit tests so there's less noise to sift through.
I hardcoded an exit code of 0 for --help since that's what Invoke returns. Not that that's being asserted, only the text.

[SuuperW]

result.Action is not null is only true for --help? That's not clear at all. Maybe that's what the comment there means to say; if so it should be explicit. (Though that's not a comment from this PR.)

If the return value isn't being asserted, and Invoke would return it anyway, ... why bother hard-coding it in the test? If you're trying to avoid having stuff actually written to the console, I think that should be handled by changing the output stream. It looks like you're already changing the output stream for testing --help anyway.

The line saying what it is parsing should probably be commented out, it looks like it's just for debug.

[YoshiRulz]

Before 2b25d09 the condition was even more cryptic, hence the comments. At least now it's being tested for.

I'm hardcoding the value so that I don't have to call Invoke to get it, and I'm avoiding Invoke so it doesn't write to console during the other unit tests, including possible future tests. It's easy to miss, but TestHelpSaysPassFlagsFirst calls RunHelpActionForUnitTest, while the others call this method with fromUnitTest: true.

Printing the input is not just for debugging, see sibling thread.

[SuuperW]

Hardcoding the expected return value based on the fact a unit test is being run should never happen.
Find another way to avoid writing to the console during unit tests.
TestConfigWithHelpOrVersion with [DataRow("--config=config.json", "--help")] is asserting a return value obtained by fromUnitTest ? 0 : result.Invoke().

Even if this wasn't the case, even if you think this is OK because this line is only encountered when --help is passed and you know what Invoke would return, it makes the code confusing and difficult to maintain.

(I disagree that it was worse prior to that commit, but let's drop the discussion of the comment line since it's outside the scope of this PR.)

[YoshiRulz]

TestConfigWithHelpOrVersion is, as the name suggests, checking that --config+--help doesn't throw (and isn't unrecognised, returning null); the exit code isn't really under test.
That branch could just as well read

// means e.g. `./EmuHawkMono.sh --help` was passed, run whatever behaviour it normally has
if (!fromUnitTest)
{
	EnsureConsole();
	_ = result.Invoke();
}
return 0;

because the early-exit of --help is known at design-time to be a success state.

If you can think of a different way to programmatically control whether it writes to stdout that's of a comparable size and complexity to my solution, then please share it.

[SuuperW]

If you can think of a different way to programmatically control whether it writes to stdout that's of a comparable size and complexity to my solution, then please share it.

I already did. Use a more generic version of RunHelpActionForUnitTest.

[SuuperW]

Why do you want this? The parser works with options and rom given in any order, even mixed.

[YoshiRulz]

See 5c73d5b#r177008277 for context. (GitHub is weird and won't show the thread until you expand the context of the first diff hunk.)

[SuuperW]

I think I agree with Morilli that it doesn't need changing. Standard behavior of Unix programs isn't relevant for a program that primarily targets Windows, any my experience tells me the current behavior of accepting the path anywhere is standard behavior. The fact that the library handles -- does not imply that it wants to be like Unix, it's just supporting a convention that some people will find useful when doing so does not harm any other users.

I would not be opposed to changing the help to state that the rom can be anywhere in command. But changing it from one correct-but-incomplete output to another correct-but-incomplete output is not useful.

[YoshiRulz]

Both orderings are accepted, see TestWithNonexistent below.

The help message could say

Usage:
  EmuHawk.exe [rom] [option...]
  OR
  EmuHawk.exe [option...] [rom]

but that seems redundant. So if I had to pick, I'm picking the conventional one every time.

[SuuperW]

The help message could say

Usage:
  EmuHawk.exe [rom] [option...]
  The ROM path may appear before or after any or all options.

[SuuperW]

I think this would be better as a more generic method, that would support any future tests of other arguments.

[YoshiRulz]

I could pass in [ "--help" ] from the unit test, but the only flags where we need to intercept and check what's being written to console are that and --version, and I don't see a reason to test --version.

[SuuperW]

Would a more generic method for all tests not avoid the issue of writing to the console during unit tests? As mentioned above, I think you can and should get rid of the fromUnitTest parameter.

[YoshiRulz]

I'm saying that a generalisation of this method would still only be applicable to --help and --version and actually just that, since it's a stripped-down version of ParseArguments, where all the logic is.

[YoshiRulz]

If you're asking about this line

BizHawk/src/BizHawk.Client.Common/ArgParser.cs

Line 208 in 2a9a111

if (!fromUnitTest && args.Length is not 0) Console.Error.WriteLine($"parsing command-line flags: {string.Join(" ", args)}");

it's so us command-line users can see the expanded version of what we've passed, in case it contains a variable or subshell, or a wrapper script has rewritten your flags. It's logged to stderr so it's not captured by a redirect/pipe.

edit: example

$> emuhawk-monort-local "$temp/Contra (USA).nes" 
(capturing output in /home/yoshi/Documents/BizHawk/output/EmuHawkMono_last*.txt)
libpng warning: iCCP: known incorrect sRGB profile
parsing command-line flags: --config=config.json /home/yoshi/Downloads/roms/debugging//Contra (USA).nes

[SuuperW]

it's so us command-line users can see the expanded version of what we've passed, in case it contains a variable or subshell, or a wrapper script has rewritten your flags. It's logged to stderr so it's not captured by a redirect/pipe.

I'm not convinced it belongs here. (you can see what your shell expands things to manually whenever you want, you don't need EmuHawk to do that for you, and I see no value in making the output captured by a redirect/pipe differ in the info given to the user normally)
But if you find it useful for everyday use, I won't make a fuss about this minor weird thing EmuHawk is doing. And if that were the only use of fromUnitTest then I think it would be acceptable.