Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow DotNetHostPath to contain env vars #3858

Merged
merged 7 commits into from
Aug 11, 2022
Merged

Conversation

Therzok
Copy link
Contributor

@Therzok Therzok commented Jul 15, 2022

This is useful for controlling the paths to the dotnet host path when you're running dotnet test src/tests/TestProjectA.csproj to avoid duplicating the runsettings.

This is useful for controlling the paths to the dotnet host path when you're running `dotnet test src/tests/TestProjectA.csproj` to avoid duplicating the runsettings
@@ -43,6 +44,12 @@ public static void FixRelativePathsInRunSettings(XmlDocument xmlDocument, string
{
FixNodeFilePath(resultsDirectoryNode, root);
}

var dotnetHostPathNode = xmlDocument.SelectSingleNode(DotnetHostPathXPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are failing because SelectSingleNode is likely case sensitive?

Copy link
Member

Choose a reason for hiding this comment

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

No test is failing on CI. Is there any change you would have forgot to push?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check previous CI runs, the problem was with the casing. If I used <RunConfiguration><DotnetHostPath> instead of DotNetHostPath it would break.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! We are planning on improving the way we read/write values in runsettings so I guess this could be handled here.

@Therzok Therzok changed the title Allow DotnetHostPath to contain env vars Allow DotNetHostPath to contain env vars Jul 16, 2022
@nohwnd
Copy link
Member

nohwnd commented Jul 18, 2022

Thanks for the PR. Please add a test that uses the new feature, so we don't lose it later.

@Evangelink
Copy link
Member

@Therzok Could you add some tests (as mentioned by @nohwnd)?

@Therzok
Copy link
Contributor Author

Therzok commented Aug 2, 2022

Yes, sorry, I completely forgot about the PR :D

@Evangelink
Copy link
Member

No worries and thanks for the remaining work.

@Therzok
Copy link
Contributor Author

Therzok commented Aug 2, 2022

I've modified an existing test to handle this scenario.

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

LGTM! @nohwnd For a second review.

@nohwnd
Copy link
Member

nohwnd commented Aug 3, 2022

Probably an edge case, but would you expect environment variables that are defined in the runsettings (or from commandline via -e) to be also usable and expandable here?

@Therzok
Copy link
Contributor Author

Therzok commented Aug 3, 2022

Didn't even know that's possible. I just followed the logic that applied to ResultsDirectory, and made it also work on DotNetHostPath :D

The way I wanted to integrate it is via msbuild setting the environment variable (some common props file) from a test project, so it could support different nesting inside a solution directory layout.

<!-- this is imported via some shared props -->
<DotNetHostPath>build/bin/App</DotNetHostPath>
src
- Implementation
- - Implementation.Tests
- Tests

Currently, with the relative path, if you use the solution as an argument to dotnet test it works, but when you pass in a project, it breaks, as it tries to probe into directories relative to the project.

@nohwnd
Copy link
Member

nohwnd commented Aug 11, 2022

Let's leave that suggestion for later then.

Thanks for the PR, merged.

@nohwnd nohwnd merged commit 311e76b into microsoft:main Aug 11, 2022
@Therzok Therzok deleted the patch-1 branch August 11, 2022 12:56
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