Resolve differences between NUnit Console and NUnitLite implementations of @filename

[CharliePoole]

@CharliePoole commented on Mon Oct 31 2016

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 commented on Tue Nov 01 2016

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 commented on Tue Nov 01 2016

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 commented on Tue Nov 01 2016

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 commented on Sat Nov 05 2016

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 commented on Sat Nov 05 2016

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 commented on Sat Nov 05 2016

Agreed to all points.

---

@CharliePoole commented on Sat Nov 05 2016

Replicating the issue for nunit-console

[CharliePoole]

@NikolayPianikov I'm trying to understand your intention in making the ArgumentsFileParser injectable in ConsoleOptions. The tests all use SimpleFiIeParser, which merely returns the lines, but this breaks any tests I add where more than one argument appears on a line.

On the surface, it seems like we would always want to parse out the arguments completely. Is there a reason for this I'm missing? Or is it something that had a purpose at one time and no longer does?

[NikolayPianikov]

@CharliePoole I've added class "ArgumentsFileParser", that actually does parsing (ArgumentsFileParserTests - unit tests).
Also I've changed "ConsoleOptions". The tests for this class are located in the "CommandLineTests" and I've added test "ArgumentsFromFilesTests" to the same file.

I would like to test behavior related to my changes in isolation too, but I did not find the usage of mock framework in this test assembly, thus I decided to add simplest implementation of parser as "SimpleArgumentsFileParser" ONLY for testing purpose as well as VirtualFileSystem. Actually I would prefer to use some mocking frameworks for it.

Summing up:

• class ArgumentsFileParser, unit tests: ArgumentsFileParserTests (has no mocks)

• method ConsoleOptions.Expand, unit tests: CommandLineTests.ArgumentsFromFilesTests (mocks: VirtualFileSystem, SimpleArgumentsFileParser)

If you want to change how files are parsed from text file to list of strings you should change only "ArgumentsFileParser" and "ArgumentsFromFilesTests".

If you want to change how we find new files with arguments or how parsed string arguments from the files will be applied as command line arguments you should change only "ConsoleOptions" and "CommandLineTests."

[CharliePoole]

So I guess you broke this out into a separate file and created a simple "mock" because you wanted to separate the concerns into different classes. It's reasonable, but not how it is done NUnitLite, where the separation is only into different methods of the same class. I will have to decide whether to keep this difference or change one of the two.

[CharliePoole]

We have usually used NSubstitute for mocks. That would require targeting .NET 3.5 for the tests although the console itself could still target 2.0. We have done that it some other situations

[oznetmaster]

I also use NSubstitute for mocks with NUnit.. I ported the current release to CF, so works with all of my users test on CF as well.