Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix test publishing#12280

Merged
weshaggard merged 2 commits intodotnet:masterfrom
weshaggard:FixTestPublishing
Oct 3, 2016
Merged

Fix test publishing#12280
weshaggard merged 2 commits intodotnet:masterfrom
weshaggard:FixTestPublishing

Conversation

@weshaggard
Copy link
Copy Markdown
Member

This pulls in dotnet/buildtools#1081 as well as changes all TestAllProjects is called. This now allows for us to independently build and test.

@stephentoub this is the overall problem we were seeing with CodeDom tests being skipped. The reason they were skipped is because during the build of the tests they were publish some things under a "tests/netcoreapp1.0" directory (in particular the test library itself) and during the test run the rest of the stuff (i.e. the test runtime etc) ended up publishing to "tests/netcoreapp1.1" and we try to run it from there and when it did the CodeDom.Tests library wasn't in that directory and thus it didn't find any tests to run. It boiled down to TestTFM being set different when building vs running and the publish step straddling both. This buildtools changes makes it so that build doesn't need TestTFM at all which should be the case because that is only interesting for the running of the tests.

cc @ericstj @joperezr

Copy link
Copy Markdown
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Excellent. Thanks for debugging and fixing this, Wes. What was special about the CodeDom test library that we weren't seeing this affect other test libraries?

@weshaggard
Copy link
Copy Markdown
Member Author

What was special about the CodeDom test library that we weren't seeing this affect other test libraries?

What was special is that it was the only library that only had a netcoreapp1.1 build configuration in this .builds file and not a netcoreapp1.0 (or default configuration).

@stephentoub
Copy link
Copy Markdown
Member

What was special is that it was the only library that only had a netcoreapp1.1 build configuration in this .builds file and not a netcoreapp1.0 (or default configuration).

Gotcha. Separate from this change, is it valid/worthwhile changing its builds file to just be the default?

@weshaggard
Copy link
Copy Markdown
Member Author

That is already part of @joperezr's PR #12241

@stephentoub
Copy link
Copy Markdown
Member

Ah. Cool. Thanks.

Copy link
Copy Markdown
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

LGTM

@weshaggard
Copy link
Copy Markdown
Member Author

My changes to dir.traversal.targets changed the default behavior when you do a build of a traversal project such that it no longer builds and runs tests by default, which is why all the legs failed. I can easily fix this for our root build but before I do that fix I wanted to get some opinions on what people think the default target should be for .builds files? Should it be just Build or should it be BuildAndTest? @stephentoub what is your thoughts and expectations when you call build on a .builds file without passing in a target?

@stephentoub
Copy link
Copy Markdown
Member

@stephentoub what is your thoughts and expectations when you call build on a .builds file without passing in a target?

My mental model, right or wrong, is that the builds file lists all the different ways the project would be built. So if I did "msbuild /t:rebuild blah.builds", I'd expect it to rebuild the project with each entry in the build file. If I did "msbuild /t:rebuildandtest blah.builds", I'd expect it to rebuild the project with each entry in the builds file and run the tests for each with the appropriate settings for each. Etc.

@weshaggard
Copy link
Copy Markdown
Member Author

For now I'm going to just limit this to the buildtools fix and leave the traversals file alone. I do think we need to give some more thought to the defaults build vs running of tests when working with the .builds file. @joperezr when you work on the documentation for that scenario we should see if what we have now makes sense or if we need to make tweaks. Currently we don't even support running all the different configurations when you do buildandtest on a .builds file, but I agree it is something we should figure out how to try and tackle.

@joperezr
Copy link
Copy Markdown
Member

joperezr commented Oct 1, 2016

FWIW, I found really useful that building the .builds file also runs the tests for the default testfm. I think that building the .builds file succesfully should be enough for you to be confident of that build passing on the CI without having to pass in any properties or targets, which is the case now but that would change if test is not the default anymore.

This is primarily to pull in change
dotnet/buildtools#1081
We no longer need PrepareForRun to go before our Test target
and in fact it causes issues with the ordering of ProjectReference
evaluation so we need to remove it from our traversal targets.
@weshaggard weshaggard merged commit a5ab071 into dotnet:master Oct 3, 2016
@ericstj
Copy link
Copy Markdown
Member

ericstj commented Oct 4, 2016

LGTM

@weshaggard weshaggard deleted the FixTestPublishing branch November 9, 2016 17:51
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Update build tools to 901-01

This is primarily to pull in change
dotnet/buildtools#1081

* Remove PrepareForRun from the target list when running Tests

We no longer need PrepareForRun to go before our Test target
and in fact it causes issues with the ordering of ProjectReference
evaluation so we need to remove it from our traversal targets.


Commit migrated from dotnet/corefx@a5ab071
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants