In nunit3-console you cannot pass parameters containing ';' because they always get splitted about nunit-console

[rprouse]

I think --testparam or -runparam would be fine.

To the whole not breaking the API and SemVer, I think it is a nice ideal and we shouldn't introduce subtle hard to diagnose behavior changes, but I think we should be free to break things in obvious ways in order to fix things. To me, incrementing the version to 4.0 to do that isn't realistic or practical and I don't think the majority of 'dark matter' developers even realize the significance of it. If our experience moving from 2.x to 3.x is anything, developers will still complain that you moved their cheese even if you're yelling the breaking changes to the world 😄

I find these long back and forth discussions a bit overwhelming and hard to keep track of especially for such a small issue. We have enough work keeping up without adding to it ourselves.

[CharliePoole]

PR #155 did not fix this issue but was closed automatically because it contained the comment "Fixes #23"

As indicated by its title, PR #155 removed parameter parsing from the framework and made it the responsibility of the runner. Additionally, as part of the change, we started using a dictionary rather than a string to represent parameters. The intent was to make it easier to subsequently fix the problem called out by this issue.

There have been various views of backward compatibility raised here. For the record, my own is that there is never a backward compatibility problem when something broken is fixed. Of course, there can always be somebody who came to rely on the broken implementation, but frankly that's there own fault. In this case, the only risk to worry about is if we use a non-standard escaping mechanism, which would not be known in advance to users. Let's not do that!

I'm re-opening this issue.

[ChrisMaddock]

I've removed the easyfix label from this - I think it's anything but! 😄

[ChrisMaddock]

I wonder if we should introduce --runparams or something with the new behaviour, and deprecate --params. The console could show a warning whenever --params is used, and we could remove it in a future release. That would provide a clean 'upgrade path' for people - the only issue is that we end up with a slightly less sensible command line argument.

--runparams would then error with pre-3.5 frameworks.

[CharliePoole]

@jnm2 Note that you can't use -rp, as that would be equivalent to -r -p. It would have to be --rp, just as you do with git merge master --ff. This is a linux/unix standard that has been adopted by many cross-platform programs and is enforced in Options.cs.

What you say about the command-line as API is correct, in the strictest sense. But we have never been very strict about it in the past, having changed our definition of command-line options many times without a qualm. The fact is that such a change on our part requires a lot less effort for users to respond to than a change in how an assertion or an attribute works.

I'm not convinced that strict adherence to definitions is always useful. I guess "What's most useful at this moment?" would be my guideline rather than "What constitutes an API?"

I do think this stuff needs to be discussed, so thank you for that.

Regarding frameworks... NUnit has generally been able to work with all it's older frameworks over many years. Each 2.x runner worked with all preceding frameworks for many years. and then many years more with all 2.x frameworks. NUnit 3.x continues to work with 2.x, through an extension.

However, in the past, the assumption was that major releases only happened every decade or more. Since you are talking about 4.0 coming out soon, then I don't think it can be the big bang change that we had with 3.0. All this is covered in our Vision document, but may not be spelled out as well as it should be. It will be up to you guys (Core Team) after I leave to decide if you want to change the Vision and Values of NUnit, but IMO you should formally change them before you act on those changes. Up to now, our goal has been that no user has to recompile using a new framework in order to use a new runner, although they may want to in order to use new features.

[CharliePoole]

@jnm2 Quite confusing, I'd say!

I'm not sure that such a feature would be any easier than what we can do already. Plus, we would still have to handle all other (non NUnit 3) frameworks differently. Better to have one approach that works for all. Seems to me that the concept of a driver is what gives us the capability to "recognize" and give an error for anything the framework we are driving won't handle. The driver is the only part of the engine that knows what framework it is using and what it is capable of.

[constructor-igor]

several samples:

• --p:A=B --params:A1=B1
  
• --p:A=B;A1=B1
  
• -p:par2=3;4
• -p:par2='3;4'
• -p:par2="3;4"
  
• -p:par2=3;par3=4
  

from documentation:

Probably, documentation should be updated from

--params|p=PARAMETER

to

--params|p:PARAMETER

Please, could you explain possible solution of the issue? Add quotes?

[constructor-igor]

@CharliePoole Please, could you explain the issue? I don't see SW issues.

[CharliePoole]

@constructor-igor Well,I thought you illustrated the problem. In -p:par2=3;4 tue user wants par2 to be set to the string "3;4" but it doesn't happen. Instead, the full string is parsed as two different settings: "par2=3" and "4" with the second one being in an incorrect format.

The creator of the issue wants to be able to have a parameter value with a semicolon in it. We would have to give him some way to escape the semicolon if we want to allow this.

[CharliePoole]

I originally thought it might already be escapable using quotes, but your examples proved that to be wrong.

[constructor-igor]

nunit3-console help shows, that semicolon and quote are used as special characters.

Possible options:

1. Disable parameter's value with semicolon.
2. Add quote for parameter's value: p:parameter="1;2"
3. Take parameter's value as is (including semicolon): p:parameter=1;2

All options have limitations.
Probably, let's choose option 3?

[CharliePoole]

@constructor-igor the special characters you marked are colons, not semicolons.

[constructor-igor]

@CharliePoole please, could you inform me your decision: what options should be implemented?

[CharliePoole]

@constructor-igor Of the options you listed...

1. This is equal to saying we won't fix the issue - it's what we already do, giving an error message. The original poster asked for us to fix it and we accepted it as a bug, adding it to the backlog. So, our intent is to fix it if we can find a way.
2. That seems reasonable to me if it will not interfere with other uses of quotes on the command-line for any operating system. One possibility is to allow either single or double quotes.
3. I can't figure out how you would do that.

My inclination is to go with either single or double quotes. It makes it a bit complicated because you can no longer simply use Split. Instead you have to scan for a semicolon that is not inside a quote. Not rocket science however and similar to what we do when scanning for test names in TestNameParser.GetSeparator

[oznetmaster]

In my CF build, I went with the standard escape syntax ('\x') for the semi-colon. Quotes (both single and double) have all the nesting issues associated with them.

I also changed to an IList for the value passed from the CommandLineOptions (same as InputFile and TestList).

[oznetmaster]

This was a very simple change in CommandLineOptions:

                     parameters = parameters.Replace ("\\\\", "\x01").Replace ("\\;", "\x02").Replace (';', '\x00').Replace ('\x01', '\\').Replace ('\x02', ';');
                     foreach (string param in parameters.Split (new[] { '\x00' }))

Note that this supports the necessary escaping of the '' as well (which is only required if it occurs before a separating semi-colon).

[oznetmaster]

As a side note, my preference would have been to go with option 3, and require a --p for each parameter (taking everything after the = as the parameter value), but I decided to remain compatible with the existing syntax.

[CharliePoole]

@oznetmaster As you rightly point out, we have to remain compatible with what we already released in 3.5

[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]

I have not back-ported this to the regular because it was unclear which approach was desired.

As I said above: "I also changed to an IList for the value passed from the CommandLineOptions (same as InputFile and TestList)."

However, there is no way to retain compatibility with existing runners, because the current interface uses a semi-colon to separate the parameter when the are passed to the runner. Even if the single string is retained, it will require a different parameter delimiter (I used a null at first). Since it was incompatible anyway, I decided to change to an IList, which was in keeping with all the other lists of strings that are passed to the runner.

[CharliePoole]

I understand that there is no way to retain compatibility if the feature is used. However, the framework currently expects options[FrameworkPackageSettings.TestParameters] to be a string, which it then interprets. I read your notation that you had changed CommandLineOptions, but I'm asking what you are passing to the framework as part of the options (in the call to Load). If it's no longer a string, that's a much bigger compatibility break.

[oznetmaster]

That is what I am talking about. Even if it is passed as a string, if the delimiter between parameters is changed, it is a breaking change.. If nothing is changed, and the parsing of the escaped semicolon must be done in the runner, it is a breaking change. There seems to be no way to pass the parameters compatibly with the existing semi-colon delimited string, and still allow escaped semicolons.

I can see no way to fix the bug without it being a breaking change.

[CharliePoole]

If it's passed as a list, then an exception is thrown whenever you use the --params option against an older framework version.

If it's passed as a string, with a major change in format, such as a different delimiter, then you'll get a different kind of error each time --params is used.

If it's passed as a string, which only changes when there's a semicolon present, then there will only be an error in those cases - exactly where we already have an error.

I think that the category of "Breaking Change" is too wide to be useful here - it's in the details.

[pixelbadger]

@rprouse - is this definitely done? Looking at #155, I can't see any changes that will support semicolons in parameters. Specifically, CommandLineOptions.cs still splits on semicolon:

foreach (string param in parameters.Split(new[] { ';' }))

[jnm2]

Ah, the plan was to decide if semicolons could be escaped without breaking people or if it would just be best to make you specify -p for each one. The latter option feels cleaner conceptually to me than escaping.

However, -p is short for --params, not --param. We could add a --param option which does not split. That would not break users of --params. Then a breaking change in v4.0 could remove --params and remap -p to --param, forcing people to move to specifying one parameter per -p .

[jnm2]

Okay, this is the only thing preventing us moving to nunit-console instead of vstest! The only use we have for parameters so far is integration connection strings.

In an ideal world, I'd write:

-p:"connectionString=Data Source=x;Initial Catalog=y" -p:name=value;without=spaces

And have it be parsed as:

TestContext.Parameters["connectionString"] = "Data Source=x;Initial Catalog=y";
TestContext.Parameters["name"] = "value;without=spaces";

Currently it gets parsed like this:

TestContext.Parameters["connectionString"] = "Data Source=x";
TestContext.Parameters["Initial Catalog"] = "y";
TestContext.Parameters["name"] = "value";
TestContext.Parameters["without"] = "spaces";

@CharliePoole what is the best path forward for this? Reading what you say about backward compatibility above, are you okay with a breaking change in a minor release?

[jnm2]

NUnit framework 3.5 and earlier are incapable of ever receiving parameters containing a semicolon.

Should we do this?

	Console current	Console vNext
Framework 3.0 - 3.5	Old behavior	Old behavior
Framework 3.6+	Old behavior	New behavior

Or require a minimum framework version for Console vNext?

	Console current	Console vNext
Framework 3.0 - 3.5	Old behavior	Error
Framework 3.6+	Old behavior	New behavior

And what should vNext be, 4.0 or 3.x?

[CharliePoole]

@jnm2 I'll need to refresh my memory - perhaps tonight. I thought this was done.

[jnm2]

@CharliePoole (Quick summary) What is done is that the console has started actually parsing the parameters to a string dictionary. It serializes and sends the dictionary as TestParametersDictionary to the framework along with the unparsed TestParameters which was only kept for compatibility with frameworks 3.5 and earlier.

What remains to be done is to change the console runner's parameter parsing logic to allow parameters to contain semicolons. We did not change this at the same time as #155 because #155 was not a breaking change. This will be a breaking change whether or not you use an escaping scheme.

Since it's a breaking change whether or not you use an escaping scheme, I'd like to push for an ideal syntax which does not require escaping semicolons. (#23 (comment)) (We could actually avoid it being a breaking change if we were willing to add a new command line argument, but that's complexity we'd carry forward with us.)

[CharliePoole]

This will be just a quick comment before my morning coffee - I'll come back again after with more....

In the past discussion about this issue, there was a lot of talk of breaking changes, with different meanings by different people. For me, the breaking change to be avoided was for the console runner to no longer work with older versions of the framework. We fixed that. Older versions will continue to work just as they always did, bugs and all.

Taking your example:

-p:"connectionString=Data Source=x;Initial Catalog=y"

If we want this to be treated as, connectionString = "Data Source = x; Initial Catalog = y" then we are changing behavior. Some people consider this a breaking change. I do not.

It's a general problem we have when we talk about something being "breaking" or not. Fixing any bug may be seen as a breaking change by a user who has come to rely on that bug. We would not be able to fix anything if we worried about that. So, to my mind, "changing observable behavior" is a necessary but not sufficient condition for a breaking change.

I think the true definition of a breaking change has to include some level of judgement on our part. What did we intend in designing the API? Is it working as intended?

Off the top of my head - I'm still waiting to have coffee here - I would say that any solution that makes it impossible to combine multiple parameters in one option is a breaking change. A solution that makes it possible to distinguish whether the user intends a semicolon to separate parameters is not necessarily a breaking change even if it requires the user to change their command-line in order to use a later version of the framework.

Back soon!

[jnm2]

By breaking I meant that after the upgrade, the same command line will either cause obvious failure (best-case scenario) or cause a silent change in meaning which goes unnoticed (worst-case scenario).

If we escape using any standard or convenient escaping sequence, will likely cause both kinds of breaking change. That goes for \;, ;;, ^;, ';', ";", `:`, %;%, $:$, etc. If we escape using an inconvenient escaping sequence like {semicolon yo}, we're stuck with something unpleasant or at least illogical and hard to remember.

If we didn't have the past to worry about we wouldn't even consider making it necessary to escape semicolons in the first place. That's just not something people normally do. Command lines have a set of rules about escaping " and \, and shells like cmd and powershell each have their own set of rules for escaping. We don't need yet another level of processing to worry about when putting a connection string in a command line.

If our choice is between a) something that causes both obvious failure and unnoticed changes in meaning, and is unpleasant and hard to remember, and b) something that causes both obvious failure and unnoticed changes in meaning, and is intuitive, I think b) is the way to go.

c) Something that does not cause failure or changes in meaning, but is confusing and extra load on both us and consumers: add a new CLI argument used either like -p:c=a;b --no-param-split or like -p2:c=a;b

[CharliePoole]

Yes, your definition is the usual meaning of "breaking change": something breaks! Elaborated very clearly, however. I'll call that breaking change (lower case) as opposed to Breaking Change meaning something that the NUnit Project will not ever do except in a major release.

Our standard is much higher, otherwise every release would be a major release.

An obvious exception is any bug, which some user has come to rely upon. We fix the bug and increment the second or third element of the version. To the particular user who was relying on that bug it's a breaking change, but to us it's not a Breaking Change. The user made a mistake in taking the behavior caused by a bug as correct.

A less obvious situation is something that we merely intended to work a certain way but which, as implemented, works a different way. Assuming both ways of working are reasonable, it's not quite the same level of bug. The user may not have been acting unreasonably in assuming things would always work the way the code was implemented. However, given our intention, it's still a bug and we have made changes in situations like that before. Usually we list them on the page of breaking changes but don't increment the major version, which shows that they are breaking for the user but not "Breaking" for us.

This case is even more borderline. We probably made a mistake in using ';' as a delimiter. It's the delimiter we use in all our multiple part options (--result for example) but this option allows arbitrary text between the semicolons, which is quite different.

The only point I wanted to make is that there is nothing cut and dry about this decision. It's going to come down to our judgement of what will be the best compromise.

[jnm2]

@nunit/engine-team Can we discuss? I'd like to get an implementation started as soon as we reach consensus.

[ChrisMaddock]

My thoughts:

1. I'd like to avoid a --params2 - but I'm struggling to see a clean way to avoid subtle breaking changes without. I'd prefer a --params2 to a --no-param-split.
2. #23 (comment) - On your solution above - I agree this is the ideal, but I don't think we can do that without this being a breaking change for everyone currently using multiple parameters, split by a semi-colon, can we?
3. #23 (comment) - On the oldbehaviour vs error - I think I prefer the error over differing behaviour between framework versions. This of course would only be thrown in the specific case where a user was attempting to pass testparameters with semi-colons, correct?

Regardless, I'd be a little worried that a user currently -p:a=hello;b=world would upgrade their framework and console, and not anticipate this to now be parsed differently. Hopefully that would be obvious in test failures, but it's not such a nice solution.

@jnm2 - What's your preferred solution, on the basis that we're not in an ideal world, and have to at least give some thought to backwards compatibility? 😄

[jnm2]

On your solution above - I agree this is the ideal, but I don't think we can do that without this being a breaking change for everyone currently using multiple parameters, split by a semi-colon, can we?

Correct. The meaning of their tests will change either obviously or silently.

On the oldbehaviour vs error - I think I prefer the error over differing behaviour between framework versions. This of course would only be thrown in the specific case where a user was attempting to pass testparameters with semi-colons, correct?

I prefer the error too. I'm not sure how conditional it should be. I was imagining that it would just never work with old framework even if you used no test parameters at all (in case you start using them and passing ;) but that does seem unnecessary. I'm not sure which is most helpful to the end user ultimately.

What's your preferred solution, on the basis that we're not in an ideal world, and have to at least give some thought to backwards compatibility?

Sadly my preferred solution is a little selfish. 😁 Break all the things!
However the solution I currently think is right for everyone is to make the breaking change, yes, but do it in a semver-compliant way which requires us to wait till v4.0. People who see the major version number change know that everything is up for grabs and that they had better read the changelog thoroughly. That means also waiting for the bigger discussion on what should go in 4.0 and how far off it is.

And because I'm going to hazard a guess that 4.0 will not land this year 😀 I'd ask for us to consider a workaround. The only workarounds that I can think of that are contractually legal to implement in 3.x (according to semver and our documented public API) are --params2 or --no-param-split.

[jnm2]

As a counter argument to myself, I might be able to be selfish and feel okay about it if there is consensus that we can do a 180 on our documented public API in a minor release and put a large message at the top of the release notes warning everyone that there are breaking changes.

But then again, if we feel so freely about such things, why not move to v4.0 next month and call it a day. That would be more representative of reality. 😄

[jnm2]

But then again, if we feel so freely about such things, why not move to v4.0 next month and call it a day. That would be more representative of reality.

Not to mention, that would neatly solve the new-runner-with-3.5-framework error scenario.

[CharliePoole]

@jnm2 We have never been strict about following semver, so I'm wondering why it is suddenly such an issue. In my view, changing how a command-line works is not the same as changing our programming API. It's a minor inconvenience so long as it's done in a way that makes it obvious to the user.

If we want to become semver strict-constructionists, we should discuss that in the core team because I think that all our core projects should follow the same approach on versioning. If we have such a discussion, I'll be proposing we abandon semver at least in the distinction between bugs and enhancements.

All that said, I'm wondering why you all see escaping like \; as a breaking change at all. That was our initial judgement and why this was marked as an easyfix.

That said, I don't see why we couldn't use --runparams, which is also a fairly easy fix.

Finally, a word on major version changes for the console. I believe any major release would still have to work with all versions of the framework, which is independent.

[jnm2]

@CharliePoole Well, and that's great. Just be sure the communication is clear to NUnit's consumers on what to expect, whatever scheme we use. Ideally that itself should be documented, with brief reminders inserted in each release notes.

The command line interface is an application programming interface, whether you type by hand or if (like me) you are scripting against it. It's a protocol that guarantees certain outcomes for certain inputs. I don't see how that's different in spirit or importance from an IL API.

All that said, I'm wondering why you all see escaping like \; as a breaking change at all.

My original post that went into this in depth is still around somewhere but suffice it to say:
-p:foo=3;bar=C:\test\temp\;baz=42

I believe any major release would still have to work with all versions of the framework, which is independent.

I was under the impression that 3.x runner did not work with 2.x framework. Is this not the general plan?

[jnm2]

@ChrisMaddock

I wonder if we should introduce --runparams or something with the new behaviour, and deprecate --params. The console could show a warning whenever --params is used, and we could remove it in a future release. That would provide a clean 'upgrade path' for people - the only issue is that we end up with a slightly less sensible command line argument.

--runparams would then error with pre-3.5 frameworks.

I like all of this.

I even kinda like the name to be honest. -rp|--runparams or -tp|--testparams.

[CharliePoole]

@ChrisMaddock I'd like to see us go with your proposal, with some mods.

Using --runparams seems to imply multiple params, bringing up once again the question of how to delimit them. How about --runparam with only one parameter per option?

I think we need to decide on a general strategy for parameters that are not acted on by a particular framework. Up to now we have simply ignored them. If we want to do some error checking for this parameter, I think we should do it in a standard way we design.

Bearing in mind that the console runner (a) doesn't know what framework will be used and (b) may be running tests that use several different frameworks together, I don't think the console can do it. That leaves two choices:

1. The framework handles it by throwing an exception.

2. The driver does it by either throwing an exception or marking the result invalid.

[jnm2]

How confusing would it be to still say -p and --params are deprecated, but to introduce --param instead?

I think we need to decide on a general strategy for parameters that are not acted on by a particular framework.

Wouldn't it be cool if the framework had a feature where it could receive errors as well as strings for each option?
--runparam would set TestParametersDictionary properly and also set TestParameters to an error message. Then framework 3.5 and lower would throw the error message when trying to read TestParameters, and framework 3.6 and above which ignores TestParameters would not be affected.

Too late now of course, but next time...

[ChrisMaddock]

--runparams was just a name I used as an example - I wasn't proposing it as the actual name. 🙂 I think that should have a some thought! I agree --params vs --param would be pretty confusing!

I've wondered before if the engine should only pass settings to the framework that the framework should be able to handle. (I believe it current passes all framework and engine settings?) That way the framework could just error on any unrecognised settings.

Of course, that would solve future problems, rather than this one here.

[jnm2]

I like --testparam a tiny bit more but also --runparam.

It sounds like the driver will have to be where the decision is made because we can't change framework 3.5 and the driver is the only place where the framework version is available? What are the pros and cons of throwing vs. marking invalid?

[CzmQlik]

Hi all,

are there any news regarding this issue? Our use case is passing a connection string as test param.

Thanks

[CharliePoole]

@nunit/engine-team I recall we fixed the framework side of this. Was the console side never fixed?

[ChrisMaddock]

I don't believe so 😞

[CharliePoole]

Bummer! I re-read all the discussion and I see it's yet another iteration of "what do we consider to be a Breaking Change?" It seems like our failure to agree about how to fix this led to nothing getting done. As I already gave my views and I'm no longer involved, I'll step out of the discussion.

[jnm2]

Ay... I'm not happy with my posts two years ago. I'm really sorry for making it complicated, everyone.

To me, --testparam stresses that the parameter is targeted at/consumed by test code. I don't have a strong opinion either way though.
I really like Chris's suggestion above of showing a deprecation warning in the console when -p or --params are used.

I'm still using testParam.Replace("{semicolon-workaround}", ";") in my test code, so I'm motivated to work on this. Would you like me to submit a PR for 3.11?

[ChrisMaddock]

Sounds good to me! 😄