Skip to content

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

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.

Sign up for GitHub

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

Open
YoshiRulz wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed638b6
Add a couple unit tests for `ArgParser`
YoshiRulz Feb 13, 2026
d0e78b1
Improve `ArgParser`'s handling of unrecognised flags
YoshiRulz Feb 14, 2026
2a9a111
Intercept output of `--help` to correct the usage line
YoshiRulz Feb 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 51 additions & 11 deletions src/BizHawk.Client.Common/ArgParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

using System.Collections.Generic;
using System.CommandLine;
using System.CommandLine.Help;
using System.CommandLine.Invocation;
using System.CommandLine.Parsing;
using System.IO;
using System.Linq;
using System.Net.Sockets;

Expand All @@ -15,10 +18,30 @@ namespace BizHawk.Client.Common
/// <summary>Parses command-line flags into a <see cref="ParsedCLIFlags"/> struct.</summary>
public static class ArgParser
{
private static readonly Argument<string?> ArgumentRomFilePath = new("rom")
private sealed class HelpSedAction(HelpAction stockAction) : SynchronousCommandLineAction
{
DefaultValueFactory = _ => null,
Description = "path; if specified, the file will be loaded the same way as it would be from `File` > `Open...`; this argument can and should be given LAST despite what it says at the top of --help",
public override int Invoke(ParseResult parseResult)
{
var outerOutput = parseResult.InvocationConfiguration.Output;
using StringWriter innerOutput = new();
parseResult.InvocationConfiguration.Output = innerOutput;
var result = stockAction.Invoke(parseResult);
var dollarZero = OSTailoredCode.IsUnixHost
#if true
? "./EmuHawkMono.sh"
#else //TODO for .NET Core: see https://github.com/dotnet/runtime/issues/101837
? Environment.GetCommandLineArgs()[0].SubstringAfterLast('/')
#endif
: Environment.GetCommandLineArgs()[0].SubstringAfterLast('\\');
outerOutput.Write(innerOutput.ToString().Replace("EmuHawk", dollarZero)
.Replace("[<rom>...] [options]", "[option...] [rom]"));
return result;
}
}

private static readonly Argument<string[]> ArgumentRomFilePath = new("rom")
{
Description = "path; if specified, the file will be loaded the same way as it would be from `File` > `Open...`",
};

private static readonly Option<string?> OptionAVDumpAudioSync = new("--audiosync")
Expand Down Expand Up @@ -134,8 +157,9 @@ private static RootCommand GetRootCommand()
RootCommand root = new($"{
(string.IsNullOrEmpty(VersionInfo.CustomBuildString) ? "EmuHawk" : VersionInfo.CustomBuildString)
}, a multi-system emulator frontend\n{VersionInfo.GetEmuVersion()}");
root.Add(ArgumentRomFilePath);
root.Options.RemoveAll(option => option is VersionOption); // we have our own version command
var helpOption = root.Options.OfType<HelpOption>().First();
helpOption.Action = new HelpSedAction((HelpAction) helpOption.Action!);

// `--help` uses this order, so keep alphabetised by flag
root.Add(/* --audiosync */ OptionAVDumpAudioSync);
Expand Down Expand Up @@ -163,6 +187,7 @@ private static RootCommand GetRootCommand()
root.Add(/* --userdata */ OptionUserdataUnparsedPairs);
root.Add(/* --version */ OptionQueryAppVersion);

root.Add(ArgumentRomFilePath);
return root;
}

Expand All @@ -177,34 +202,43 @@ private static void EnsureConsole()

/// <return>exit code, or <see langword="null"/> if should not exit</return>
/// <exception cref="ArgParserException">parsing failure, or invariant broken</exception>
public static int? ParseArguments(out ParsedCLIFlags parsed, string[] args)
public static int? ParseArguments(out ParsedCLIFlags parsed, string[] args, bool fromUnitTest = false)
Copy link
Contributor

@SuuperW SuuperW Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@YoshiRulz YoshiRulz Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@SuuperW SuuperW Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@YoshiRulz YoshiRulz Feb 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@SuuperW SuuperW Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

@YoshiRulz YoshiRulz Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@SuuperW SuuperW Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
parsed = default;
if (args.Length is not 0) Console.Error.WriteLine($"parsing command-line flags: {string.Join(" ", args)}");
if (!fromUnitTest && args.Length is not 0) Console.Error.WriteLine($"parsing command-line flags: {string.Join(" ", args)}");
var rootCommand = GetRootCommand();
var result = CommandLineParser.Parse(rootCommand, args);
if (result.Errors.Count is not 0)
{
// generate useful commandline error output
EnsureConsole();
result.Invoke();
if (!fromUnitTest) result.Invoke();
// show first error in modal dialog (done in `catch` block in `Program`)
throw new ArgParserException($"failed to parse command-line arguments: {result.Errors[0].Message}");
}
if (result.Action is not null)
{
// means e.g. `./EmuHawkMono.sh --help` was passed, run whatever behaviour it normally has
EnsureConsole();
return result.Invoke();
return fromUnitTest ? 0 : result.Invoke();
}
if (result.GetValue(OptionQueryAppVersion))
{
// means e.g. `./EmuHawkMono.sh --version` was passed, so print that and exit immediately
EnsureConsole();
Console.WriteLine(VersionInfo.GetEmuVersion());
if (!fromUnitTest) Console.WriteLine(VersionInfo.GetEmuVersion());
return 0;
}

var unmatchedArguments = result.GetValue(ArgumentRomFilePath) ?? [ ];
if (unmatchedArguments.Length >= 2)
{
var foundFlagLike = unmatchedArguments.FirstOrDefault(static s => s.StartsWith("--"));
throw new ArgParserException(foundFlagLike is null
? "Multiple rom paths provided. (Did you delete half of a flag? Or forget the \"--\" of one?)"
: $"Unrecognised flag(s): \"{foundFlagLike}\"");
}

var autoDumpLength = result.GetValue(OptionAVDumpEndAtFrame);
HashSet<int>? currAviWriterFrameList = null;
if (result.GetValue(OptionAVDumpFrameList) is string list)
Expand Down Expand Up @@ -268,11 +302,17 @@ private static void EnsureConsole()
openExtToolDll: result.GetValue(OptionOpenExternalTool),
socketProtocol: result.GetValue(OptionSocketServerUseUDP) ? ProtocolType.Udp : ProtocolType.Tcp,
userdataUnparsedPairs: userdataUnparsedPairs,
cmdRom: result.GetValue(ArgumentRomFilePath)
);
cmdRom: unmatchedArguments.LastOrDefault()); // `unmatchedArguments.Length` must be 0 or 1 at this point, but in case we change how that's handled, 'last' here preserves the behaviour from the old hand-rolled parser
return null;
}

internal static void RunHelpActionForUnitTest(TextWriter output)
Copy link
Contributor

@SuuperW SuuperW Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@YoshiRulz YoshiRulz Feb 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@SuuperW SuuperW Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@YoshiRulz YoshiRulz Feb 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var result = CommandLineParser.Parse(GetRootCommand(), [ "--help" ]);
result.InvocationConfiguration.Output = output;
result.Invoke();
}

public sealed class ArgParserException : Exception
{
public ArgParserException(string message) : base(message) {}
Expand Down
40 changes: 40 additions & 0 deletions src/BizHawk.Tests.Client.Common/ArgParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
using System.IO;
using System.Linq;

using BizHawk.Client.Common;

namespace BizHawk.Tests.Client.Common
{
[TestClass]
public sealed class ArgParserTests
{
[DataRow("--config=config.json", "--help")]
[DataRow("--config=config.json", "--version")]
[TestMethod]
public void TestConfigWithHelpOrVersion(params string[] args)
=> Assert.AreEqual(0, ArgParser.ParseArguments(out _, args, fromUnitTest: true));

[TestMethod]
public void TestHelpSaysPassFlagsFirst()
{
using StringWriter output = new();
ArgParser.RunHelpActionForUnitTest(output);
var outputLines = output.ToString().Split('\n');
var usageLine = outputLines[outputLines.Index().First(tuple => tuple.Item.Contains("Usage:")).Index + 1].ToUpperInvariant();
Assert.IsTrue(usageLine.IndexOf("OPTION") < usageLine.IndexOf("ROM"));
Copy link
Contributor

@SuuperW SuuperW Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@YoshiRulz YoshiRulz Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@SuuperW SuuperW Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@YoshiRulz YoshiRulz Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@SuuperW SuuperW Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help message could say

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

}

[DataRow("rom.nes", "--nonexistent")]
[DataRow("--nonexistent", "rom.nes")]
[DataRow("--nonexistent", "--", "rom.nes")]
[TestMethod]
public void TestWithNonexistent(params string[] args)
{
int? exitCode = null;
var e = Assert.ThrowsExactly<ArgParser.ArgParserException>(() => exitCode = ArgParser.ParseArguments(out _, args, fromUnitTest: true));
Assert.AreNotEqual(0, exitCode ?? 1);
Assert.Contains(substring: "Unrecog", e.Message);
Assert.Contains(substring: "--nonexistent", e.Message);
}
}
}