Fix for issue #1178 based on fix in PR #1214 for version 3.17++ #1394
Conversation
There was a problem hiding this comment.
LGTM thanks @OsirisTerje
Changes appear to match the original PR. Out of curiosity, why leave the old code commented instead of removing?
| // See PR 1114 https://github.com/nunit/nunit-console/pull/1214/files | ||
| engine.InternalTraceLevel = Options.InternalTraceLevel != null | ||
| ? (InternalTraceLevel)Enum.Parse(typeof(InternalTraceLevel), Options.InternalTraceLevel) | ||
| : InternalTraceLevel.Off; |
There was a problem hiding this comment.
If we want the default to be InternalTraceLevel.Off why doesn't the TestEngineActivator.CreateInstance ensure that?
Now it explicitly requires switching off.
Would this not affect other places where an engine is created (e.g. the adapter)
In the engine the level is set as: public InternalTraceLevel InternalTraceLevel { get; set; } = InternalTraceLevel.Default;
Where Default < Off.
However in its Initalize method it only tests on Off.
Instead of Default being its own state, should we not have Default = Off or drop Default completely?
if(InternalTraceLevel != InternalTraceLevel.Off && !InternalTrace.Initialized)
{
var logName = string.Format("InternalTrace.{0}.log", Process.GetCurrentProcess().Id);
InternalTrace.Initialize(Path.Combine(WorkDirectory, logName), InternalTraceLevel);
}
There was a problem hiding this comment.
Ideally yes, but I am very shaky on everything here, so I don't dare doing too much now. We have three sort of main branches, and they are very diverged. So this is just part of it, and I try to do as little as possible to make them even more diverged.
I am also very uncertain about what to do with the v4 (main) here, since it is not working, and is not finished, and I don't have enough information to know what is missing or not.
|
@stevenaw That piece of code has been ported all over the different branches in exactly that way. As said above (answer to Manfred), the branches are a mess, and I just want to keep the code changes "as is" for now. |
No description provided.