Test parameter containing a semicolon

[oznetmaster]

Currently, the --p (--params) command line option uses the semi-colon as a separator. This makes it difficult to have a parameter containing a semi-colon,

I thought that using multiple --p option would work, but internally, all of the parameters are still concatenated together using a semi-colon as a separator.

The external interface should not be changed. However, the internal format (used to pass the option from the command line to the builder needs to be changed to use non-inputtable character as the separator. Perhaps a null should be used? Alternatively, since the options value is an object, the parameters could be passed as an array of strings?

Not sure why the decision was made to simple concatenate all the parameters separated by semicolons was made in the first place?

[CharliePoole]

This is basically the same issue as nunit/nunit-console#23, applied in this case to nunitlite. Since a lot of discussion has already taken place there, let's keep that as the primary issue and port the fix here when it's done.

[jnm2]

@CharliePoole I'm not sure that this issue should be specific to the console runner at all. I'm having this issue via the VSTest adapter. I send parameters to VSTest using a .runsettings file and anything after a semicolon is removed. After reading the discussion in nunit/nunit-console#23, it strikes me that this should be able to be fixed for the VSTest .runsettings scenario without it being a breaking change.

At least part of the issue is in this repo: src/NUnitFramework/framework/Api/DefaultTestAssemblyBuilder.cs#L138

This is an incredible pain because the only use case I have for parameters is integration script connection strings 😆

[CharliePoole]

@jnm2 I was aware of that and probably should have recorded it as such in the first place. I think any general solution will involve some sort of escaping by each individual runner and then interpretying that escaping in the DefaultTestAssemblyBuilder.

As is often the case, the NUnitLite solution is the easiest to work on first because the client is in the same repository as the framework.

BTW, any general solution will probably have to deal with escaping = as well as ;.

[oznetmaster]

I am pretty sure this was fixed in nunitlite already. Unless it was part of my PR that was not accepted, but I think not.

It should be checked.

[jnm2]

What work is left to do?

[CharliePoole]

Nothing has been done on this in nunitlite. Also, I don't it in PR #1863.

It's probably not quite the "easyfix" for a newbie, but should be easy enough for @jnm2 if you want to implement it. 😄

[jnm2]

I'd much rather leave up to the individual command line or adapter how to parse the parameters and whether to escape, and change options[FrameworkPackageSettings.TestParameters] to an IDictionary<string, string>.

That way I can have the vstest adapter simply populate a dictionary and not have to escape just right to be parsed just right. The current parsing code would move to nunitlite and nunit.console and the CLI escaping can be implemented there separately, or left as-is.

[jnm2]

I also see tremendous value in a custom NUnitOptions type which exposes strongly typed properties like TestParameters. If necessary it could also implement IDictionary<string, string>.

[jnm2]

And here's the other loose end: https://github.com/nunit/nunit3-vs-adapter/blob/c8e99e6aefb64a0fb71d2ee2fb3405978aa4c766/src/NUnitTestAdapter/NUnitTestAdapter.cs#L193

[oznetmaster]

I proposed a fix for this quite a while ago, but it changes the TestParameters in CommandLineOptions to an IList, and it was considered a breaking change.

I changed it that way in my CF builds, and it work fine. However, I am not constrained by the issue of breaking changes :(

[oznetmaster]

Also, there is no need to escape = because it can only occur on the RHS of the required =, and therefore is not ambiguous.

[jnm2]

@oznetmaster Yes, I read that in nunit/nunit-console#23.

= would need to be escaped if it appeared in a key name. Silly, but that's how it should be. My plan is to avoid having to escape and unescape in the first place.

What are all the things that would break if the type was changed to IDictionary<string, string>?