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

Add test for '--help' option parser #56

Closed
3 tasks
chshersh opened this issue Aug 7, 2022 · 5 comments
Closed
3 tasks

Add test for '--help' option parser #56

chshersh opened this issue Aug 7, 2022 · 5 comments
Assignees
Labels
cli-options CLI options, parsers good first issue Good for newcomers hacktoberfest https://hacktoberfest.com/ test

Comments

@chshersh
Copy link
Owner

chshersh commented Aug 7, 2022

It would be nice to test that the --help option by default outputs what we want. And it's also nice to see what changes when we change default parsers.

So I propose to add a test that parses the --help option and compares it with the expected output of this option.

The plan is:

  • Add new module Test.Iris.Cli where the new test will live
  • Extract cmdParserInfo from Iris.Env to Iris.Cli
  • Write a unit test for the --help option using execParserPure from optparse-applicative
@CThuleHansen
Copy link
Contributor

CThuleHansen commented Sep 16, 2022

Hi. So I have a looked a bit on this and I have a question.
"Extract cmdParserInfo from Iris.Env to Iris.Cli"
does this mean to move cmdParserInfo into a file Iris/Cli/ParserInfo.hs within the module Iris.Cli.ParserInfo?
And pass it as an argument to mkCliEnv?

@chshersh
Copy link
Owner Author

@CThuleHansen You understand the required changes correctly with only a single change 😌

  • Instead of passing cmdParserInfo as an argument to mkCliEnv, let's make cmdParserInfo accept CliEnvSettings. And then we can pass all the settings from mkCliEnv to cmdParserInfo

This way, in tests, we can construct ParserInfo using cmdParserInfo by specifying our test settings.


As I write this, I realise that it'll require to move CliEnvSettings into their own module Iris.Settings to avoid cyclic dependencies. But I'm fine with this, let's do this 😌

@CThuleHansen
Copy link
Contributor

CThuleHansen commented Sep 19, 2022

Okay, so I am approaching the target at https://github.com/CThuleHansen/iris and here is a small status and a few questions.
I have extracted and moved some bits according to our discussion, but I also moved cmd to Settings.hs, which seems off. But I need to access it from ParserInfo, so it is to avoid cyclic dependencies.
Question: Is a Types.hs file needed?

When I run my test, then --no-input parses successfully. However, --help gives a parserFailure.
I am thinking this is because --help is baked in and not added manually, whereas --no-input is not.
The output for --help seems correct besides the ParserFailure:
image

Question: What to pass for ParserPrefs in execParserPure? These are not very well documented in https://hackage.haskell.org/package/optparse-applicative-0.17.0.0/docs/Options-Applicative-Extra.html#t:ParserPrefs
I have used Opt.ParserPrefs "suffix" False False False OptTypes.Backtrack 80 False False 0
What is prefMultiSuffix? For the others, I just took default.

I am not getting the option for version and numeric-version because I have not passed any version settings yet.
This might fix the issue with help as well. So I do not have any questions for this as of yet :)

@chshersh
Copy link
Owner Author

I have extracted and moved some bits according to our discussion, but I also moved cmd to Settings.hs, which seems off. But I need to access it from ParserInfo, so it is to avoid cyclic dependencies.
Question: Is a Types.hs file needed?

I see the dilemma. Putting Cmd inside the Settings modules feels off to me as well. In that case, let's put it into a separate module Iris.Cli.Cmd. It's a part of CLI so it makes sense to keep it under a separate module. The data type is small at the moment but it'll get bigger. And putting it into a separate module will enable to write more detailed documentation on it 😌

I'm generally against generic modules like Types.hs, Helper.hs, Utils.hs, Common.hs and etc. So I'd like to avoid such modules in my projects. They tend to become a kitchen-sink of everything. It's worth thinking about a more appropriate place to put generic stuff in.

However, --help gives a parserFailure.
I am thinking this is because --help is baked in and not added manually, whereas --no-input is not.
The output for --help seems correct besides the ParserFailure

Indeed, this is the optparse-applicative-specific behaviour. The --help option is implemented ParserFailure to stop parsing immediately and output the result of --help. In that case, in tests for the --help option, we should expect ParserFailure and match the expected text with the hardcoded text of --help.

Question: What to pass for ParserPrefs in execParserPure?

The Iris.Env module uses the execParser function to parse the CLI arguments. I see from its code that it uses the defaultPrefs value. So I propose to use it in the tests as well.

@CThuleHansen
Copy link
Contributor

CThuleHansen commented Sep 21, 2022

Pull-request created: #68

I'm generally against generic modules like Types.hs, Helper.hs, Utils.hs, Common.hs and etc. So I'd like to avoid such modules in my projects. They tend to become a kitchen-sink of everything. It's worth thinking about a more appropriate place to put generic stuff in.

Yes, they certainly do.

The Iris.Env module uses the execParser function to parse the CLI arguments. I see from its code that it uses the defaultPrefs value. So I propose to use it in the tests as well.

I should have found this, thanks :)

@chshersh chshersh moved this to In Progress in Iris Project Sep 22, 2022
CThuleHansen added a commit to CThuleHansen/iris that referenced this issue Sep 22, 2022
CThuleHansen added a commit to CThuleHansen/iris that referenced this issue Sep 23, 2022
@chshersh chshersh added the hacktoberfest https://hacktoberfest.com/ label Sep 27, 2022
Repository owner moved this from In Progress to Done in Iris Project Sep 28, 2022
@chshersh chshersh unpinned this issue Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli-options CLI options, parsers good first issue Good for newcomers hacktoberfest https://hacktoberfest.com/ test
Projects
No open projects
Status: Done
Development

No branches or pull requests

2 participants