Using new adapter related settings from runsettings of testplatform

[navin22]

• Added to code to read DisableAppDomain, DisableParallelization and DesignMode.
• Respective parameter changes in the adapter code based on above setting.
• Added related unit tests.

[CharliePoole]

It looks good to me provided we decide how to resolve the potential conflict between DisableParallelization and NumberOfTestWorkers.

[CharliePoole]

Should we account for the possibility that the user has used the NumberOfTestWorkers setting directly? Cases are:

DisableParalllelization	NumberOfTestWorkers	Action
true	0	Disable
true	> 0	Conflict - error message?
false	0	Disable
false	> 0	Conflict - see note

NOTE: Currently we don't distinguish between the setting being missing and being set to false. Since that's the case, I would be inclined to allow parallel execution in the fourth row. If we were to distinguish the two, then I'd give a message if DisableParallelization were explicitly set to false while NumberOfTestWorkers was positive.

[navin22]

Will take action only if DisableParallelization is true. If not will not take any action.

DisableParalllelization	NumberOfTestWorkers	Action
true	0	Disable
true	> 0	Conflict - error message? [Is Warning Better than error message?]
false	0	Disable [Will not take any action, since NumberOfTestWorkers is already set]
false	> 0	Conflict - see note [Will not take any action, so it will be parallel]

[OsirisTerje]

If DisableParallelization is false, and NumberofTestWorkers > 0 then we have parallel. There is no conflict there.
(@charlie: Did you switch the values there for DisableParallelization in the Note?)

For the 2nd case, DP = false and NOTW>0, I think it makes sense to make DP take precedence when true, and disable Parallel. A message can be ok, but could also be set to display only for Verbosity>0. All these messages are annoying in build outputs.

[CharliePoole]

You're right Terje, I switched two cases. I'm moving this discussion to the general comments because it gets hidden as a line comment when changes are made.

[CharliePoole]

Repeating/Expanding/Correcting my table here for discussion...

DisableParalllelization	NumberOfTestWorkers	Action
true	< 0	Disable, since user specified DP and didn't specify NOTW
true	0	Disable, since user specified both settings consistently
true	> 0	Conflict, since user specified both settings inconsistently. [Conflict1]
false	< 0	Do nothing
false	0	Conflict, since user specified NOTW to disable but set DP false [Conflict2]
false	> 0	Just pass the NOTW setting
unspecified	< 0	Do nothing
unspecified	0	Disable by passing the NOTW setting
unspecified	> 0	Just pass the NOTW setting

I expanded the table to include DP not specified. That's different from an actual false setting, IMO, although I don't know if we can actually tell the difference. It depends on how the .runsettings values are created, of course.

Conflict1:
I'm not clear as to whether this should be an error (stop execution) or a warning. The problem seems to be that the same setting may appear because the host added it OR because the user edited the .runsettings file. If it were guaranteed that it only came from the vstest command-line, then it makes sense for it to be an override. If a user actually creates a file with conflicting values, then I usually favor stopping the entire show. We could live with a warning message, however, and users would probably grow accustomed to that behavior. Documentation could specify that setting DP true overrides NOTW.

Conflict2:
In this case, it's clear that the user put two conflicting settings in the file. I think we could go with either a warning or an error that stops the run. Of course, this assumes we can distinguish between an actual setting of false and no setting being specified.

Note that we use a negative setting of NOTW to conventionally indicate that the setting was unspecified. In that case, we don't pass anything to the framework, which uses its defaults.

[navin22]

runsettings can be passed by the users, so we can't guaranty that fields are only populated by the platform.

And we can differentiate if DisableParallelization is specified or not using nullable bool. If specified, it will be honored and it will override the NumberOfTestWorkers. If not specified no change (current logic will take care of it).

We should decide on whether to stop the flow in case of conflict or continue with warning or verbose log ?

[CharliePoole]

I think we should differentiate between DP not specified and specified as false but only if we intend to do something different in each case. If not, we should keep it as a simple bool. IMO, deciding how we want to handle each situation should come before coding changes.

After looking at all the options, I'd be on the side of simply documenting that DP overrides NOTW. I think we should log a warning in case NOTW has been set to a positive value because we know that had to come from the user.

It might be a good general principal to say that general settings in the .runsettings file override our own adapter-specific settings when in conflict. That has the advantage of being easy to explain.

[navin22]

@CharliePoole: Yes we want to differentiate between DP not specified and take no action. And if specified override if there's a conflicting NOTW and log warning.
It's good to document this, will update the runsettings documentation.

[OsirisTerje]

@navin I agree with @CharliePoole that we should only differentiate between not specified DP and DP==false if we decide to do something different in those two cases. I don't see any reason to make those two situations different.
That is: Given DisableParallelization == true, it overrides NOTW and there is no parallelization. If NOTW >0 and only if verbosity >0 we log a warning. This opens the possibility to use the DP as a quick way of turning off parallel.
Given DisableParallelization == false (which is also default if not specified, and DP is bool), the NOTW rules and controls parallelization.
This makes sense to me also because the word DP is a negative, and I would then expect it to Disable, but not expect it to forcibly Enable.

[CharliePoole]

I think @OsirisTerje has summarized it well. I'd only warn that IIRC we only honor verbosity==1 if a debugger is attached.

[navin22]

@OsirisTerje then it means that NOTW settings will be overridden only if DP is true. Otherwise ignore. Even if it's specified and has false as value?

[OsirisTerje]

@navin22 Yes.
@CharliePoole We got to check out the verbosity too later. It's a good way of reducing noise and enabling details when needed.

[navin22]

@OsirisTerje: Changed the code to override NOTW only when DA is true.
Please take a look at the code.

[OsirisTerje]

Ok, LGTM now. Merging