Skip to content

request to remove "unrecognized args" panic from test_runner #16522

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.

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

Closed
wants to merge 5 commits into from
Closed

request to remove "unrecognized args" panic from test_runner #16522

wants to merge 5 commits into from

Conversation

mscott9437
Copy link
Contributor

@mscott9437 mscott9437 commented Jul 24, 2023

given this simple args tester:

test "hello" {

std.debug.print("\n", .{});

const args = try std.process.argsAlloc(allocator);
defer std.process.argsFree(allocator, args);

std.debug.print("ARGS::{s}\n", .{args});

}

in order to run the test and pass random arguments, i have to use a custom test runner which is a copy of the default test runner with the panic removed. I run the command with:
zig test args.zig --test-cmd-bin --test-cmd hello --test-runner .\test_runner_custom.zig

with my edits to the code (the same edits i made for the custom test runner), i can narrow down the command to use the builtin test runner:
zig test args.zig --test-cmd-bin --test-cmd hello

It seems to me like having to use a custom test runner just to pass command line arguments to a test is overkill. I do a lot of testing with random inputs both from stdin as well as cmd args, so it should be more useful if this was part of the default test runner, if possible. I only removed the else statement within the loop checking for the "--listen=-" parameter. After that I recompiled and ran the tests with no apparent issues. I can't imagine what problems allowing unspecified command line arguments in the test runner would cause, since any extra data should be discarded anyway, but I could be wrong. Also this is my first pull request here, so if there's anything I'm not seeing or understanding as to why this might not work, I would be happy to look into that as well.

@matu3ba
Copy link
Contributor

matu3ba commented Jul 24, 2023

Could you please 1. explain for what use case you would like to ignore cli args, 2. the used cli arguments and 3. justify how omitting them is not an error?

The test runner has intentionally only necessary arguments to keep it small and simple to remain correct and simple to invoke manually.

@mscott9437
Copy link
Contributor Author

mscott9437 commented Jul 24, 2023

  1. Just to clarify, I am not actually ignoring command line args. By making the change to the code below it is simply allowing the existing "--test-cmd" parameter to accept any unspecified value to construct the command for running the testing binary compiled by the test builder. So any other args with existing handlers (i.e. "--listen=-") will still be recognized. And since all arguments have to be passed using the existing zig test parameter "--test-cmd" anyway, I am not actually making any changes to what parameters can be accepted into the "zig test" command itself, so I don't see any added complexity here. For example it's not like you will be able to run "zig test example.zig --randomarg1 --randomarg2". Rather you are using the existing parameter "--test-cmd" to pass additional arguments into the compiled test binary ( --test-cmd randomarg1 --test-cmd randomarg2 ), which is specific for each individual project, and in normal cases the custom program itself should be allowed to do its own checking of arguments.

  2. I am only using the existing zig test parameter "--test-cmd" to pass the values to the test binary. the values I'm passing could be anything.

  3. I am not omitting anything that I am aware of. The default test runner here is only checking for one argument "--listen=-", which will still be included and processed normally.

The use cases for this are as many as there are which involve passing command line arguments into an application. In my case I am writing a simple query language to get information out of a database, so naturally for testing purposes it will be easier to pass random queries into the command line to check the results. A more typical scenario would be to aid in CLI tool development, where it would be necessary for testing to pass different arguments as parameters to check that the output of the custom application is expected and error handling is working normally.

In short it doesn't make sense to me that you cannot test some core feature of an application, especially if that's the whole reason you are writing the application to begin with. I understand there is the option of a custom test runner, but it seems to me that something like a custom test runner should be reserved for more edge use cases, since it involves adding an extra source file to the project at an early stage. The default runner should be able to handle typical use cases (i.e. cli tool parameters).

Finally, if it's not going to work then I would only request to be provided a solid example of how this would cause an issue, as well as possibly a workaround which doesn't involve using a custom test runner. Thank you for your consideration 🙂

@matu3ba
Copy link
Contributor

matu3ba commented Jul 24, 2023

and in normal cases the custom program itself should be allowed to do its own checking of arguments.

The whole point of unit tests is that they are ideally independent from external input, which includes cli arguments. The only exception to this is a manipulation of the environment, for example to start a vm or qemu with the program inside, and those things are done by the frontend (keeping the unit test runner as simple as possible).

I do agree insofar, as test_runner is not an ideal name and the description could clearly state something like

//! Default test runner for unit tests.

at the top most position.

I think that is the reason why we disagree and/or misunderstand another.

@mscott9437
Copy link
Contributor Author

Now I see what you are getting at. Because the default test runner is the one being used for the builtin unit tests. My first thought would be that unit tests should also be able to test for user input, but from the way you are explaining it then that would already be too high level of a concept for testing functions. In fact it would probably fall into the category of an edge case, which I mentioned previously and would thus necessitate a custom test runner. So I will go ahead and close this and just move my args logic into the main function of a "testing" app. This will also give me a good foundation to go further into the source code later on and see how everything works or what issues there might be. Thank you for clarifying all of that! I was working on something but got sidetracked with this issue which was a huge distraction for me. So finally I will be able to get back to what I was originally doing. See you next time!👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants