Resolve differences between NUnit Console and NUnitLite implementations of @filename

[CharliePoole]

We (well actually I) mistakenly merged the nunitlite changes thinking that the differences between the console runner and nunitlite implementations had already been resolved between @NikolayPianikov and @oznetmaster

We need to decide first what features to include / exclude then change one or both of them so that users do not get confused and features we have not decided to support do not become supported by default.

Eventually, we should also resolve code differences for the common features but that can await the point where we create a shared codebase for conmmand-line option processing.

I'm putting this in the discussion pipeline so that the NUnit team can work out what we want to support. Everyone else is welcome to participate in the discussion as well.

[NikolayPianikov]

I've used Regex from @oznetmaster and I could add the nesting of any levels during 5 minutes if it is needed. I agree it would be better to have one code base and it doesn't matter for me which code will be preferable. I am just interested in implementation of this feature.

So I would like to highlight some questions:

About requirements:

• We should clarify a list of requirements.
• Should we implement minimum sufficient set of requirements? Or ...
  • Will we support the nesting?
  • Will we support symbol " within the argument?

About implementation:

• Should we duplicate a code or should we use some common assembly?
• Which code we will choose?
• Who will be responsible for implementation?
• Is it possible to use our Linq in the implementation (additional reference)?

[CharliePoole]

I support @oznetmaster 's proposal in PR #1856:

"I guess I am recommending we leave at least one level of nesting (currently my code has two - it is only a constant), and remove the alternative delimiter. Then modify the console code to support the same syntax."

I think this is a minimum fix that will keep users from getting confused.

As already stated, I don't support trying to make the code common right now. There's a bigger project I would like to do that would include this: developing an entire shared codebase for option handling, including our own version of Options.cs and using a submodule to share the code. But we have many more high priority things to worry about right now, so I think this can wait. So long as the two bits of code actually do the same thing, we will avoid complaints from our users.

[CharliePoole]

I intended this to be in the Discussion pipeline when I created it but didn't put it there.

@NikolayPianikov Once we are done discussing and have decided on a course of action, we'll move it to the Backlog. Someone will decide to do it or we will pick someone. Then they will do it. 😄

[CharliePoole]

OK... I don't see any action on this folks, so I'll lay out a proposal...

Let's allow a single level of nesting and remove the alternative delimiter as @oznetmaster suggested.

If this is agreed upon, then we need to

1. Add a single level of nesting to NUnit3-console.
2. Remove extra nesting level from NUnitLite
3. Remove alternative delimiter from NUnitLite

@rprouse @oznetmaster @NikolayPianikov Do you agree?

[oznetmaster]

The number of levels of nesting in my code is a constant. It is only there to eliminate the possibility of infinite recursion. It serves no other function.

Removing the alternative delimiter is easy.

[rprouse]

Agreed to all points.

[CharliePoole]

Replicating the issue for nunit-console

[CharliePoole]

@oznetmaster I looked in the repo for your code, but didn't see it. I guess you are talking about your private build. Did you retain compatibility with external runners? That is, do you still pass a single string to the framework containing the parameter info, or are you passing a list?

[oznetmaster]

@CharliePoole not sure what you are asking. This issue is about @filename, not parameters

[CharliePoole]

My mistake - I had both issues open.

[NikolayPianikov]

I agree with it

[CharliePoole]

The issue is duplicated for nunit-console as nunit/nunit-console#126.

Both issues are available for somebody to self-assign.

[CharliePoole]

As indicated in the earlier comment, the following differences exist between the two implementations:

1. Console runner is line based. Each line in the file represents one argument and no parsing is done except to look for a comment symbol ('#') in the first position. NUnitLite is basically token based. Tokens are separated by a separator, which defaults to \n. The token can be changed to allow multiple items per line. This is the most fundamental difference between the two.

2. NUnitlite supports nesting, limited to a depth of 3. Console runner does not support it.

Design:

1. We should use the simpler line-based approach in both cases. This won't break @oznetmaster 's implementation, because it is separate. As for our own users, the feature has never been documented.

2. As proposed earlier, we should add nesting to the console runner.

I'll leave a brief period for comment.

[JustinRChou]

I thought the alternative delimiter in nunitlite is going to be removed too?

[CharliePoole]

If we use the line-based approach, as I'm proposing, then there are no tokens and therefore no delimiters. So there is no need for an alternative delimiter.

This is essentially what I laid out back in November. It's just that nobody picked up the ball and implemented it, so I thought I should say it one more time and then just do it.

[oznetmaster]

As I pointed out in November, if the line-based approach is used, then the file inserted with the @ option cannot, for instance, contain a --where option that actually spans multiple lines, perhaps one condition per line.

That is a very useful capability, like writing fluent asssertions on multiple lines.

[NikolayPianikov]

Console runner is not only line based, for example see ArgumentsFileParserTests:
[TestCase("--arg1 --arg2", "--arg1\n--arg2")]
It uses the same tokenizer expression like NUnitlite see ArgumentsFileParser

There is only one difference as I see: console runner works without a recursion - I can add this by several lines of code if it is needed. But if we want to combine these approaches it is other task

[CharliePoole]

@NikolayPianikov I guess I'm using the term in a very limited way. What I mean is that ArgumentsFileParser handles each line separately. That line may, in fact, contain multiple tokens but as far as I see it's not possible for a token to extend across multiple lines.

This was a design restriction I originally placed on the implementation. I felt it would give us a very simple pre-processor without restricting things too much. @oznetmaster is arguing otherwise: that we need to allow a single argument to extend across lines of text. I wasn't convinced before but I'm beginning to see that there could be such a need, so let's continue the discussion.

[CharliePoole]

@oznetmaster I see your point. For test name selection, it's simple to just use multiple --test options. But for other criteria or a combination of test names and other criteria, the ability to span multiple lines would be of some benefit. For example...

--where
    test==some long name ||
    test==another long name ||
    cat==SmokeTests

In such a case, the main reason for using a file might be to make the option more legible.

What do other folks think?

[oznetmaster]

Exactly!

[NikolayPianikov]

We could allow it if user limits each argument by symbols " and it is quite easy
Thus it should look like:
"--where
test==some long name ||
test==another long name ||
cat==SmokeTests"

[CharliePoole]

I agree, although it has some issues if the argument uses quotes internally.

At this point, still waiting for a concensus on supporting the feature. @nunit/framework-team @nunit/core-team Please don't assume that your silence will be taken to mean agreement.

[jnm2]

I'm an advocate for treating all whitespace characters equally.

[oznetmaster]

I do not line the quote approach, for exactly the reason that @CharliePoole mentioned - nested quotes.

The approach that I implemented in nunitlite has been working quite successfully for my users for almost two years now, so I know it is a viable solution.

[CharliePoole]

So how would your approach handle the example?

[JustinRChou]

Since the console runner already handles --where "method =~ /Source.*Test/ && class =~ 'My.Namespace.Classname'"
so that should also be how it should be written in a file.

[CharliePoole]

@oznetmaster I'd like to hear how the user would input those examples in your implementation.

@JustinRChou While I like your example, the fact is that there isn't a standard for how options are specified at the command-line. The OS handles it.

[CharliePoole]

@oznetmaster Still waiting to hear from you regarding how you suggest that multiple-line options, like where clauses, should be handled. It's not clear to me from your comments so far.

[CharliePoole]

I decided to add tests as a starting point. 😄

[oznetmaster]

I have tests for the my nunitlite implementation.

Do you want them?

[oznetmaster]

Actually, they are in the repo in nunitlite.tests\CommandLineTests.cs

[CharliePoole]

Yes, I started with those and am expanding them.

So far, there doesn't seem to be a clean way to handle multi-line arguments containing spaces other than putting them into quotes. I'd really like something like the cited example above to work.

[jnm2]

@CharliePoole the way I would imagine is for each argument to control consumption of the remainder of the command line. So --where's logic is that it takes the rest of the command line unless it runs into a -- not inside a string which would signify the start of a new switch. Or perhaps even -[a-z0-9].

[CharliePoole]

@jnm2
That would be the best solution if we were ready to redo our command-line processing - and maybe this will push us to that.

Currently, the handling of @filename in the command-line is done by a pre-processing step. The actual processing is done by Mono.Options, which is included as source in both NUnitLite and the console runner, but which we have made it a practice to leave unmodified.

I have a back-burner project to create our own command-line options processor. Mono.Options has features we don't need and lacks a few things we could use. I was hoping, however, to be able to fix the current issue while sticking with the pre-processor model.

One option may be a "smarter" pre-processor. Or I may just do as much as is possible within the current design constraints and deal with better stuff later.

@oznetmaster
FYI, I found and fixed a few issues in the tests themselves:

1. The expected result is tested as a string, using Join, but that doesn't allow for the fact that various string arrays can produce the same string. When I added new tests, I found that some things looked correct when joined but that the arguments were not separated correctly, especially when spaces were involved.
2. The character '|' is used with a special meaning but it's a commonly used character in the where clause. I may end up with a whole new test method just for where clauses.

[CharliePoole]

General approach I plan to use for resolving this issue together with nunit/nunit-console#126...

1. Use @NikolayPianikov 's approach to the tests. I may also move away from TestCase to use of TestCaseSource, which is cleaner when the arguments get more complicated.

2. Drop use of the [] notation to specify a delimiter that replaces newlines. Although this is useful in @oznetmaster 's extended implementation, it doesn't really serve a purpose in ours right now. If we needed something like this in the future, I'd prefer to see it implemented as special comments at the start of the included file. That way, the user doesn't have to remember a special option to use with each file. Anyway, we don't need it for the moment.

3. Explore some special syntax for use in the file when specifying multi-line arguments. This is particularly useful for TSL, which we should be able to lay out like a program in the file. May also be useful for options. I'll post ideas as I get to this part.