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

Allow users to pass arguments via file(s) instead parameters of a command line #94

Closed
NikolayPianikov opened this issue Oct 12, 2016 · 19 comments
Assignees
Milestone

Comments

@NikolayPianikov
Copy link
Member

It's important because arguments containing a bunch of assemblies with full paths have a significant length. And users should split these assemblies to run tests in several sessions or use .nunit configuration files. It could be implemented like:
--arguments=filename and -a=filename
File name contains the text similar a command line.

For example a command line could be:
nunit3-console.exe -a=myArgs
where the myArgs file contains one line:
assembly1 "assembly 2" assembly3 --noheader --teamcity --x86
or several lines:
assembly1 "assembly 2"
assembly3
--noheader --teamcity --x86

Also in the simplest implementation we can use one command line argument per line, because we could avoid the question how to deal with symbol ".

Theoretically:

  • The -a argument could be used several times in the command line.
  • Each file with arguments could contain other arguments -a
@rprouse rprouse added the Idea label Oct 12, 2016
@CharliePoole
Copy link
Contributor

In the past, I was accustomed to the syntax "@filename" to represent a file whose contents were interpolated into the command-line. Is that too archaic a convention to use today? It would be a convenient way to pre-process args before handing them to Mono.Options to parse.

I'm for this as a feature if we can agree on a syntax. I'm up for repeating it on the command-line but I don't like the idea of nesting.

@NikolayPianikov
Copy link
Member Author

NikolayPianikov commented Oct 12, 2016

I like the idea @filename

I agree that the nesting is not useful in general scenarios, but for this argument we should check a source of the argument each time and we should raise an error if the argument was defined in a file.

Also I spent some time to see how settings are working now and to write tests for settings before making changes. I found that we have no any unit/integration tests for it. I started from prototyping and found that we have no way to write "real" unit tests - we are not using any frameworks for mocks so far. It is possible to choose any and use it everywhere?

@CharliePoole
Copy link
Contributor

If we treat it as a pre-processing step

@CharliePoole
Copy link
Contributor

If we treat it as a pre-processing step, then the options code won't even understand the format and should give an error.

We have unit tests for all our settings. We don't have real integration tests for anything. Maybe you mean something else?

@CharliePoole
Copy link
Contributor

We use nsubstitute

@NikolayPianikov
Copy link
Member Author

NikolayPianikov commented Oct 12, 2016

Ok I will use nsubstitute if it is needed for tests.

I mean unit tests for OptionSet class that looks quite complex for me. Yes I found unit tests for the ConsoleOptions class.

What do you think: should we use a file's format like for the command line - the single line in a file? Or we parse the each argument from the own line?

I going to make a simple implementation to have criticism from your side.

@NikolayPianikov
Copy link
Member Author

Is the order of the arguments essential?

@CharliePoole
Copy link
Contributor

Options.cs is imported from mono, where it is tested.

In principle, we don't modify it but we made a few small exceptions to make it build on certain platforms. I'm hoping to back those changes out after we remove CF from the framework project.

FYI, I also have plans for a major revision of how we do options, eliminating the use of Mono.Options, which has some limitations for our purpose and yet is also more complex than we really need! We should only modify and test ConsoleOptions.

Suggestion: Create a static Expand method in ConsoleOptions and change the constructor to call base(Expand(args)) - I think this will do everything you need. For the file format, I would start by one per line and we can decide later if we want to do more parsing. Order is not currently significant but it probably makes sense to maintain order in case some future change demands it.

@oznetmaster
Copy link
Contributor

oznetmaster commented Oct 12, 2016

When I implemented this in my CF build (nunitlite), I used the @filename as we had discussed over a year ago [(https://github.com/nunit/nunit/issues/890)]

The reason I never backported the implementation to the full nunit framework was that we never were able to agree upon a number of issues, and also that I was unfamiliar with the nunit-console, so I needed someone to port the nunitlite implementation to it if we went that way.

I implemented two level nesting, but @CharliePoole did not want nesting. I allowed both a single line and a multi line format, with a number of options on the @filename, including specifying a delimiter for the options, and the format, but @CharliePoole thought these were too complicated.

My implementation is a preprocessor one, like the implementation of the --testlist option.

Nesting allows there to be files containing fixed sets of options that are used in every test suite to be included with test suite specific options easily.

My implementation and the tests for it has been in use for over a year in my Crestron CF build.

@CharliePoole
Copy link
Contributor

Simplicity (for users) is something we always strive for in NUnit. When given a choice between implementing what may be too little or may be too much, we choose to risk too little because features can always be added but - once added - can only be removed with difficulty. Other projects, of course, have different values and act differently.

One thing to be sure of in any of these discussions: when we are talking about complexity, we are not saying it is too hard to implement. We are referring to the complexity that gets presented to the user. Since we are programmers and so are our users, it's tempting to try to wear both hats. I'm happy to say that I got over that habit a few years ago. Let's implement the "simplest thing that can possibly work" and wait to see what the users ask for once they see that.

@oznetmaster
Copy link
Contributor

Usefulness is important as well. I found that having @filename without any nesting severely limited its usefulness, which was why I chose one level of nesting.

Having only a single line in the file, or having only one option per line, both limited the usefulness of the @filename option. The problem was specifically there was an ease of use to have all the test options on a single line, but at the same time, it was much easier to have the test to be performed listed one per line (like in --testlist). Limitng to one or the other cause a loss of ease of use.

Simplicity is not always the same as ease of use.

@CharliePoole
Copy link
Contributor

Simplicity and ease of use are somewhat moot, if no feature ever gets implemented. @NikolayPianikov 's proposal is 14 hours old. We've discussed it and settled on some features, which he is working on. That seems like good progress to me.

@oznetmaster
Copy link
Contributor

As I said, I implemented this in my nunitlite CF build over a year ago, but you were not interested in my implementation.

Backporting to full nunitlite would take about an hour. It would, however, not do anything for nunit-console.

@CharliePoole
Copy link
Contributor

@oznetmaster Although I didn't know you had implemented it privately, I'm not surprised. There has never been any doubt in my mind about your ability to implement whatever you choose to implement.

However, your implementation never arrived at our users, which was my point. Saying that was because I was "not interested" is unfortunate because it's neither true nor useful.

I'm interested in getting features to users. @NikolayPianikov made a basic proposal. I made a counter-proposal as to format, which he indicated he liked. We asked each other questions. We compromised on an approach in a matter of hours. We'll do more of that when there is a preliminary bit of code to work with. Our users will get some minimal feature set to try and then we'll look at it again in a few months and possibly give them more. This way we will minimize the risk of taking features back that prove unworkable after use. This is what happens with most of our features.

If we need to discuss the past proposal any further, let's do it by email. @NikolayPianikov created this issue to discuss his proposal, not past history.

@NikolayPianikov If @oznetmaster has code to parse multiple options out of a line in the file that he is willing to contribute, you might consider using it rather than re-inventing.

@NikolayPianikov
Copy link
Member Author

@oznetmaster @CharliePoole It would be great to use ready-made solutions, rather than inventing their new bikes. @oznetmaster could you point me which classes I could use or could you append code to parse files to the nunit3-console project (with tests if it possible) or if you prefer to implement this feature in full let me know.

@oznetmaster
Copy link
Contributor

It is interesting that my code for this in CommandLineOptions (written a year ago) looks virtually identical to the code written by @NikolayPianikov . The only difference is that mine supports recursion, and mine supports both option per line (with delimiter override) and single command line formats at the same time.

About 10 additional lines of code.

I guess there is really only one way to do this easily :)

NikolayPianikov pushed a commit to NikolayPianikov/nunit-console that referenced this issue Oct 14, 2016
… of a command line - fix according to review, support the lazy expansion
@NikolayPianikov
Copy link
Member Author

@oznetmaster Maybe because we are thinking the same way? :)

NikolayPianikov pushed a commit to NikolayPianikov/nunit-console that referenced this issue Oct 17, 2016
… of a command line - implementation allowing to parse argument with spaces and multiple elements per line
@CharliePoole
Copy link
Contributor

@rprouse @NikolayPianikov Is there any reason this should still be open?

@rprouse
Copy link
Member

rprouse commented Mar 8, 2017

Nope, the PR was just missing the fixes before the issue number so it didn't get closed.

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

No branches or pull requests

4 participants