Skip to content
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

ir-test: Improve console args handling #18

Closed
wants to merge 4 commits into from
Closed

ir-test: Improve console args handling #18

wants to merge 4 commits into from

Conversation

weltling
Copy link
Contributor

@weltling weltling commented Mar 16, 2023

The console arg parsing functionality is pulled through a dedicated class. The license is MIT which seems compatible with the IR license.

The ir-test program code is moved under misc-utils subdirectory, where the arg parsing library is added as well. The current options have been migrated to the new class, some more options have been added for a better flexibility.

Especially there are two new options to pass BUILD_DIR and SRC_DIR, that would override the corresponding environment variables if supplied. Otherwise, the env variables will be used, thus maintaining the backward compatible behavior.

The help output is automatically generated by the arg parsing library:

$ ./ir-test --help
Usage: ir-test [options]

Execute IR tests

  --show-diff                Print failed test diff to stdout
  --no-color                 Disable output colorization
  --build-dir arg (=.)       Bulid dir path. When passed, overrides the BUILD_DIR environment variable
  --src-dir arg (=.)         Source dir path. When passed, overrides the SRC_DIR environment variable
  --test-dir arg (=./tests)  Test dir path. 
  --ir-exe arg               Path to the `ir` test app. 
  -h, --help                 Display help text and exit

Unknown options would make the program bail out:

 $ ./ir-test --no-such-opt
ERROR: unknown option '--no-such-opt'
Try 'ir-test --help' for more information.

While on this change, several exception handling arguments have been constified to comply better with the other parts of the code.

Fixes: #10

@dstogov
Copy link
Owner

dstogov commented Mar 16, 2023

I'm not sure if this simplifies anything.
We add a dependency on 1000+ lines library and then code of ir-test.cxx becomes even longer.
It looks like adding few more strcmp (to support new options) is much easier.

In general, we don't need --src-dir and --build-dir options (as well as corresponding environment variables).
--ir-exe completely substitutes --build-dir, --test-dir makes the same for --src-dir.

It's better to pass a list of test directories without --test-dir, just all trailing arguments without - prefix. It's great to support not only directories but also regular files and wildcards.

e.g. ir-test --ir-exe=build_x86/ir tests/x86/add*.irt tests/debug*

I would also think about this tester program independently from IR project, especially if we move it to separate folder. Name it tester (or some better name) instead of ir-test, use --exe (or some better name) instead of --ir-exe. Later you may identify all other specific to IR things and make them configurable through command options and/or resource file (default command args [--save], test extension [irt], target [???]).

@weltling
Copy link
Contributor Author

weltling commented Mar 16, 2023

Thanks for checking this. I guess the difference is on the views with regard to the intention. As per the title here and in the linked issue, it meant improve and not simplify :) So my view was to improve the handling on the tool side - default options, error handling and so on. And on the code side, to have a simple way adding new options similarly to getopt. Walknig through argv is of course the doable way, just that it's perhaps too sparse.

New options can come, like those you mention. Some options would be most likely a secondary consequence when adding more features. Say others like diff or parallelization can be controlled from the cmdline interface. Now with what you've suggested to support glob syntax is a very nice one, but we'd have to add some implementation for that as well. Or, if we'll come to the conclusion we want to introduce more sections in test files or have some specific evaluation of the section contents .There can be other ideas, but in general that would mean adding more and more code.

Of course, taking improve as in terms of simplify, the handling would be quite different. If we go this direction and we don't have to keep the env vars, can remove some code indeed. It would be probably also not needed to move the source file anywhere, too. If i read correctly, on the simplification direction it might be a list like this then:

  • Options to be kept - --no-color, --show-diff
  • New option --exe to pass the test executable
  • Input path(s) is given as a trailing positional argument
  • Check for supporting glob. Some simple incomplete implementation might be possible, but a more extended one would need either glibc or some additional code, i guess

If renaming, maybe test-runner, or run-tests as per the analogy with the original tool?

Perhaps adding more code and depending on more code is not that bad. It's just the price for a better featured tool. Reusing such external classes as a drop-in functionality is very easy and efficient. I see your point however, that this kind of code and extensive tooling possibly doesn't belong into this repo.

Thanks

@weltling
Copy link
Contributor Author

Abandoning this one for now. The change is too extensive and leads the focus away from the main dev subject. The present functionality is good enough to serve the project. Any further functionality is to be supplied in a minimalistic manner, as the need arises.

Thanks

@weltling weltling closed this Mar 27, 2023
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.

ir-test: Improve console argument parsing
2 participants