Traits: This caches values of various environment variables which act…

[radical]

… as (#42)

.. switches to control features like logging imports. For use with
tests, this caching can be disabled but that is enabled only for DEBUG
builds. Which means that any tests trying to toggle and test those
environment variable dependent features will work only for debug builds.

So, we remove that restriction. And because the condition first checks if we are
running tests and only then looks at

Environment.GetEnvironmentVariable("MSBUILDRELOADTRAITSONEACHACCESS")

.. it shouldn't affect performance much (untested!).

[rainersigwald]

Seems fine, but I'd ideally like an RPS run to double-check that it doesn't cause a noticeable perf problem.

[AndyGerlicher]

I'm a little concerned with this one. RunningTests is cached so that's fast, but checking an environment variable to see if it should cache an environment variable look up on every call is not great. Also this could end up loading BuildEnvironmentHelper earlier which I'm not crazy about (it uses reflection and there are weird VS scenarios that we've seen in the past). This isn't a blocker but I'd rather not if we don't actually need it.

How about on first access creating the Singleton we check MSBUILDRELOADTRAITSONEACHACCESS just the first time and if it's 1 then we disable cached values for all future invocations? A new class is a bit unfortunate since it's pretty inefficient for likely a single read, but I'm fine leaving it that way. A single GetEnvironmentVariable overhead the first time is fine and I think this should solve your issue? And we should setup our tests and your tests to set that environment before running.

[cdmihai]

@radical
I don't think the MSBUILDRELOADTRAITSONEACHACCESS environment variable is actually needed anymore, since we moved to the reflection based way of detecting whether we're running in tests. Can you experiment with removing it and seeing if anything breaks?

[radical]

I don't think the MSBUILDRELOADTRAITSONEACHACCESS environment variable is actually needed anymore, since we moved to the reflection based way of detecting whether we're running in tests.

I'm not sure what you mean by that. But I noticed that we set this environment variable in TestEnvironment..ctor, so at least for all users of TestEnvironment, we don't need to check for this. Maybe we can extend that to just assume that if RunningInTests is true, then we should create a new instance, thus skipping the environment variable check completely? Is that what you meant?

And yes, removing that didn't break any tests.

[cdmihai]

Maybe we can extend that to just assume that if RunningInTests is true, then we should create a new instance, thus skipping the environment variable check completely? Is that what you meant?
And yes, removing that didn't break any tests.

Yup, I think the environment variable is obsolete. You can delete all read / write mentions to it. I remember that in the past, we relied on the env var as a way for tests to communicate to the Traits to refresh themselves. We needed the var because Traits is compiled into every assembly, so every assembly has it's own copy of the object, and we had no easy way of addressing all of them. But now we use reflection as a single source of truth, which makes me believe we don't need the env var at all.

[radical]

Updated to remove all uses of the env var.

[AndyGerlicher]

@dotnet-bot test OSX10.13 Build for CoreCLR please

This is cleaner, I like it. And hopefully there aren't issues pulling BuildEnvironmentHelper here :).

[AndyGerlicher]

@dotnet-bot test Windows_NT Build for CoreCLR please