Skip to content

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jan 7, 2019

Relates to dotnet/corefx#34385

Changes:

  • All test assets required for helix runs can now be generated as part of the test project build by settings /p:ArchiveTests=Tests|IntegrationTests|PerformanceTests|(Packages)|All.
  • Remove $(SkipTests) property which is not needed anymore as the test targets should only be used if you REALLY want to run tests.
  • Remove $(Performance) property which is not needed anymore as performance tests now have a unique target and performance related msbuild files are only imported if `IsPerformanceTestProject´ equals true.
  • Add test targets depending on the project: /t:Test, /t:IntegrationTest and /t:PerformanceTest. These are the common arcade targets that are expected to be present in the respective test project type.

@ViktorHofer ViktorHofer self-assigned this Jan 7, 2019
@ViktorHofer ViktorHofer requested a review from safern January 7, 2019 14:48
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jan 7, 2019

cc @danmosemsft @ericstj @wtgodbe

@safern
Copy link
Member

safern commented Jan 7, 2019

So with this change if I want to generate helix archives from a build script i.e for CI or official builds, I have to do build.cmd -includetests /p:ArchiveTests=true /p:GenerateTestScripts=true? Why not instead, if ArchiveTests == true then set GenerateTestScripts to true? So that way it is easier, and we don't have to remember to pass 2 arguments, rather than just one?

@ViktorHofer
Copy link
Member Author

I don't have a strong opinion on that. The reason why chose this is that ArchiveTests handles the ArchiveTests target and GenerateTestScripts handles the GenerateRunScript target. I think it's a good idea to bundle it together :)

@ViktorHofer
Copy link
Member Author

LOL actually I just saw that GenerateRunScript is already a dependency of ArchiveTestBuild.

@safern
Copy link
Member

safern commented Jan 7, 2019

So then GenerateTestScript target will be hooked together by the dependent targets 😄 that is a nicer approach.

@ViktorHofer
Copy link
Member Author

@safern ready to merge?

@ViktorHofer
Copy link
Member Author

@safern @ercistj PTAL. I think this is now ready to be merged.


<Target Name="GenerateRunScript"
Inputs="unused"
Outputs="$(RunScriptOutputPath)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why inputs outputs if you don't use them for incremental nor batching?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GenerateRunScript is part of the incremental build, at least that's what I believed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that if you have no inputs and only outputs I would expect that target to always run.

still generates the test execution script and archives the test dir for Helix consumption.
-->
<Target Name="RunTests"
Condition="'$(SkipTests)' != 'true' OR '$(TestDisabled)' == 'true'"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that this was inverted before. No-one was using it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I freaked out when I saw that condition 🤐

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea, but will follow-up on that one.

@ViktorHofer ViktorHofer merged commit beadc52 into dotnet:master Feb 21, 2019
@ViktorHofer ViktorHofer deleted the TestTargets branch February 21, 2019 00:06
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants