Updated perf targets

[ianhays]

My old targets were buggy and didn't allow perf tests to be included when building the tests but not running them. I've modified them and renamed them to better accomodate this.

I also removed the v5.0 TargetFramework declarations that are no longer required.

@mellinoe @jhendrixMSFT

[ianhays]

Goes with dotnet/corefx#4118

[mellinoe]

GitHub is having a hard time displaying the diff here, not exactly sure why. Could you try keeping the tabs at 2 spaces instead of 4 and seeing if it's easier to diff?

[ianhays]

Huh, sorry about that. Not sure why it replaced my spaces. Fixed.

[mellinoe]

I'm trying to understand the overall diff here, meaning the overall difference in behavior given different commands. You replaced $(RunPerfTestsForProject) with just $(Performance) everywhere, but the two were mostly equivalent before, modulo a few extra checks. I think the only real diff is that we copy the xunit assemblies over after "CopyTestToTestDirectory" rather than before "RunTestsForProject", and we condition the inclusion of the assembly attribute on something different. I think we could accomplish those two things with a lot less change. Perhaps we can leave everything else the same but just condition those two things on the new attribute added to the test projects themselves?

[ianhays]

Sorry @mellinoe , I didn't prioritize diff minimization at all.

I want to accomplish a few things:

• I wanted to get rid of the RunPerfTestsForProject variable; the only thing that RunPerfTestsForProject added on top of the Performance property was a check of the current OS and an outdated call to Exists that is no longer necessary.
• Minimize repeated checks. AnalyzePerfResults was moved closer to the target where it is used (in the middle of the file) where it could be combined with the TestCommandLine declaration that also depends on $(Performance). I'm completely fine with reverting this to minimize diff as it's a purely a stylistic change.
• Modify the perf copy to occur after CopyTestToTestDirectory
• Modify the Assembly inclusions to depend on the IncludePerformanceTests property. This didn't need to be moved in the file; I can revert that.

[mellinoe]

Sorry, I was just trying to understand the overall difference in behavior here the easiest, because I was thinking that the first two bullet points you listed were sort of a no-op in terms of behavior differences and were more for cleaning up the target (which is definitely a good thing).

I wanted to get rid of the RunPerfTestsForProject variable; the only thing that RunPerfTestsForProject added on top of the Performance property was a check of the current OS and an outdated call to Exists that is no longer necessary.

That seems reasonable to me. There was also the difference in that you could explicitly set RunPerfTestsForProject to 'false' in order to avoid actually running the performance tests while still building them, so it's a bit less clear to me if this is the same as before. But it seems like this property wasn't really helpful or clear, or perhaps didn't even work correctly. I think it makes more sense now in that you set Performance=true if you want to build everything for performance tests, and then you build the "Test" target if you actually want to run them. Does that sound like an accurate description of how things will work?

Minimize repeated checks. AnalyzePerfResults was moved closer to the target where it is used (in the middle of the file) where it could be combined with the TestCommandLine declaration that also depends on $(Performance). I'm completely fine with reverting this to minimize diff as it's a purely a stylistic change.

No, we should leave it, it makes more sense where it is now, especially if RunPerfTestsForProject is gone, because then it would be the only thing up there 😄. I think when I made that note above I didn't realize that RunPerfTestsForProject was removed (which is fine, I don't think it was that useful).

Overall this LGTM.

[ianhays]

Thanks, @mellinoe!

Does that sound like an accurate description of how things will work?

Yup, exactly.