Use dictionary-based property for test run parameters

[jnm2]

This issue tracks the orchestration of the work assigned to me by @CharliePoole in #1885 (comment).

• Superseding Supplementing the string-based test parameters property with a dictionary-based one. Decouples parsing test parameters between the various runners, from each other and from the framework which test assemblies happen to be built against.
• Providing backward compatibility for both old runners with new framework and old framework with new runners.

• Framework: Test parameter parsing out of framework #1942, commit cdf0604
• NUnitLite: Test parameter parsing out of framework #1942, commit 00fb62d
• nunit3-vs-adapter: Test parameter parsing out of framework nunit3-vs-adapter#289
• nunit-console: Test parameter parsing out of framework nunit-console#155
• dotnet-test-nunit: Test parameter parsing out of framework dotnet-test-nunit#96
  • nunit.portable.agent: Test parameter parsing out of framework nunit.portable.agent#20
• nunit-gui (Does not have a concept of test parameters)
• Any others? (No)

Due to the two-way backwards compatibility guarantee, each of these top-level tasks is completely independent of the others.

[CharliePoole]

This seems to be the same as #1885, which is already assigned to you. Is there a reason you prefer to use another issue or are you trying to create an Epic to cover all the work?

[CharliePoole]

OK.. I see you are calling this the umbrella issue. Good idea, just not implemented as we do it. I'll make this a Zenhub Epic when I get home. Can't do that on my phone.

[jnm2]

@CharliePoole however you manage it is of course fine. I saw this issue as a grouping of tasks, but at the same time a subtask under #1885. I guessed we'd have some meta discussion which I didn't want arbitrarily fragmented across PRs, on the more specific issue (moving the parameter parsing out of the framework), and the parent issue (fixing test parameters with semicolons) is already fairly long.

[oznetmaster]

Since this originated with #1885, is the fix for that going to follow this PR, because this does not fix that issue at all.

[jnm2]

@CharliePoole @oznetmaster Once these PRs go through, the VS adapter will be done. Then escaping and any other CLI parameter parsing enhancements can be done in each runner without affecting backwards compat, which I'm thinking will be the last task necessary for #1885. That should now be a breeze. Hooray for decoupling runner parameter parsing from test assembly framework version =)

[CharliePoole]

@jnm2 I'm going to organize this a bit. What you've done is fine but a little more complex than we needed perhaps. Are you using ZenHub? You'll be able to see the Epic and it's issues only if you are - but that's how we do it.

I'm going to make the original issue an Epic. On this issue, I'm not convinced the title is what we want to but I'll come back to the details after I catch up with you. The time-zone difference makes it a little hard to keep up!

[CharliePoole]

I'm a bit confused by the title: I don't believe we want to remove the existing parsing from the framework. For backward compatibility, it seems we have to retain it. Could you clarify what you want to do for this issue? Of course, if you do a PR it will clarify it, but it probably makes sense to decide what we want first.

[CharliePoole]

@oznetmaster I'm confused by your comment. What PR do you mean?

[jnm2]

@CharliePoole I just installed ZenHub.

I'm a bit confused by the title: I don't believe we want to remove the existing parsing from the framework. For backward compatibility, it seems we have to retain it. Could you clarify what you want to do for this issue? Of course, if you do a PR it will clarify it, but it probably makes sense to decide what we want first.

Yes, we have to retain the old string property and parsing implementation in the framework forever for backwards compatibility, but we're moving the effective (dictionary-based) parsing responsibility from now on when both the framework and runner are new.
From my perspective there is nothing more to PR for this particular issue.

This set of PRs enables you change the escaping in some runners without affecting the rest of the runners or affecting backwards compatibility with the NUnit framework version which the test assembly is referencing.

[CharliePoole]

OK... so are you saying you don't plan to fix the semicolon problem when creating the dictionary, but that somebody would need to create a new issue and PR to do that?

[jnm2]

For clarity I think the semicolon escaping fix should be in a separate PR, a child of the #1885 issue and a sibling to this issue.

The PRs in this issue are only concerned with refactoring the responsibility, opening the way to the semicolon escaping fix while keeping compatibility guarantees.

I'm more than happy to help with the semicolon escaping fix too.

[oznetmaster]

I think this whole thing has gone the wrong way, but then, I fixed it for my users a year ago. And, as I pointed out earlier, I fixed it in ConsoleOptions.

What started as a real problem that needed to be fixed has seemingly become something else.

[jnm2]

@oznetmaster This brings us backwards compatibility with old-new and new-old, plus it allows each runner to have the option of its own style.

Without doing this you'd be making the semicolon escaping fix a breaking change. You'd also be keeping runners beholden to the test assembly framework version when parsing the list of --p lines, which doesn't even make sense when there never were --p lines as in the case of the VS adapter.

[CharliePoole]

@jnm2 OK, that is definitely not how I would have split it but we can live with it. I'm going to do some retitling of your issues so they make a bit more sense to me. I'll explain it on the first one I do.

While I think we should stick with what you've done right now, in order to avoid rework, my advice is that you pay attention to how the committers tell you they prefer to receive the fixes in future. After all, each individual PR is essentially a release - first to the committers for review and then to the public once we merge. (I.E. a merge is a release for those who follow our dev builds.)

I know that the advice about how to break things down often seems interfering and burdensome for to new contributors. But those who follow that advice get their PRs - on the average - approved and merged more frequently and more rapidly than those who don't.

Another kind of advice you'll get - primarily from me or @rprouse - is about certain areas of code. It's something like "There they be dragons!" for areas of code that are known to be fragile or just hard to understand without knowing a lot of other things first. If you get that kind of advice, please don't take it as a besmirching of your skills, because it definitely isn't.

Anyway, to the question of whether the "changing of responsibility" should be separate from "fixing the semicolon" problem. Yes, it should be separate! I would separate it by having separate commits to the same PR. I'd make sure that the first commit worked before moving on. The alternative we are following here is to separate them into two PRs. That will work as well. It may not, however, get the full fix into a release. Therefore, you have to make sure and the reviewers have to make sure that each separate PR is completely standalone and will work by itself. That's about twice as much work as making sure that one PR works and will take twice as long. Because of a feature (bug?) in GitHub, you should not create a PR for B, which is based on A until after A is merged. If you do, we will end up re-reviewing A+B. You can, however, create a branch for B based off A, so long as you delay creating a PR till A is merged.

Those are the tradeoffs - you can decide how you prefer to work, paying attention to the announced release dates (milestones) for each package.

[CharliePoole]

Title Change: I think this issue (or the issues under it) are principally about adding the dictionary-based property. We are not taking parameter-parsing out of the framework. It remains for use with the old property and will even be added to when we do escaping.

Umbrella Issue: This is really more complicated than we generally like to get. We now have an Epic which includes the "umbrella" which in turn includes other issues. That's one more level of structuring than we try to use. For the moment, I'm keeping everything because I don't want to lose any links. When I'm confident that everything is referenced under the Epic, I'll probably close this.

[jnm2]

@CharliePoole I appreciate the explanation. I am open to any process that I can understand.

Here's why I thought two PRs would be helpful:

• I wasn't sure that I would be the one to implement the escaping
• I was pretty sure the escaping conversation would take a long time
• Even if the parsing move ships before the semicolon escaping ships, it delivers value on its own by solving a blocking issue for the VS test adapter.

But that's just one guy's perspective. Would you like me to wait these PRs until the semicolon discussion is finished and implemented? That's fine with me.

We are not taking parameter-parsing out of the framework. It remains for use with the old property and will even be added to when we do escaping.

I still protest that framework parameter parsing should not be added to. I do not see a way to touch the framework parsing without breaking old-new or new-old. I think it should be frozen and treated as opaque, a compatibility implementation detail behind setting the dictionary.
If you decide that this back-compat framework parsing should be changed, you cause breaks. If you cause breaks, I'm going to rewind to the part of the conversation where I think it's worlds better to cause a complete break than an infrequent or subtle one. I'd petition to implement the complete break by reusing the "TestParameters" key for the dictionary.

What can be added to is what this set of PRs provides, a way to change the parsing in the runners instead, and retaining compatibility. If we only touch the parsing in the runners, the semicolon problem is fixed for anyone using new-new. Anyone using old-new or new-old will have the behavior they've always had.

[CharliePoole]

@jnm2 Those are all good reasons and you're probably right that the discussion could go long. At some point, an arbitrary decision may be needed.

For the moment, I would suggest concentrating on the things that block what you want to use: the adapter plus the framework. One problem with implementing similar code in multiple projects is that you have multiple updates to make if the reviewers ask for changes so seeing one set through completely may prove more satisfying to you, thereby keeping you working on the project longer. 😄

In the current schedule, the framework release is January 7, console is January 14 and the adapter is January 21. Ideally, we should at least get the dictionary stuff merged before those dates.

Regarding improving the in-framework parsing, I believe it can be done but that is a separate discussion. I may have to write some code we can talk about in order to communicate the idea better. I'd love to see it in the new release but that may not be possible.

[jnm2]

For the moment, I would suggest concentrating on the things that block what you want to use: the adapter plus the framework.

Is there anything you're waiting on me for right now?

[CharliePoole]

Completion of the PRs for nunit and nunit-console, and then on to the adapter.

[rprouse]

This is merged here in the framework and PR's are outstanding everywhere else, so I am going to close this so it gets tracked with the 3.6 release.

[jnm2]

I answered my remaining two tasks' questions since the answers look clear to me. Feel free to correct me if I'm wrong.

[CharliePoole]

@jnm2 I'm sorry to say that this issue got so complex, with so many comments and questions, that I don't know what those two remaining questions were. 😞 Please ask again if you need to.

[jnm2]

@CharliePoole The last two tasks (checkbox items) at the top of this thread: #1941 (comment)

[CharliePoole]

Answers in parentheses are right.