-
Notifications
You must be signed in to change notification settings - Fork 998
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
Helix proof of concept for unit tests #224
Conversation
…tep for debugging
… into dev/advolker/helix
Big shoutout to @AdamYoblick for his work on this earlier and also @alexperovich for his assistance as well as support on the Arcade bits |
Waiting on #537 |
@alexperovich would you mind reviewing these changes to get our unit tests on Arcade? We did most of this at the end together, so just looking for that little green check mark ✔️ 😉 |
} | ||
|
||
ProcessStartInfo startInfo = new ProcessStartInfo(); | ||
startInfo.FileName = Path.Combine(BinPath(), byPathFromBinToExe.Trim('\\')); | ||
startInfo.EnvironmentVariables["DOTNET_ROOT"] = dotnetPath; | ||
startInfo.EnvironmentVariables["DOTNET_ROOT"] = dotnetPath; // required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding the reason why this is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only I remembered 😆 ... I'll figure it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is why you want to explain comments like this one :). Maybe for the integration tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to verify the impact on #514 before this is merged. Requesting a deferral on merging until 13 March 2019 so I have time to review and adjust if necessary.
<PropertyGroup> | ||
<AccessibilityPackageVersion>4.6.0-alpha-27122-5</AccessibilityPackageVersion> | ||
<XUnitVersion>2.4.1-pre.build.4059</XUnitVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With java applications we usually had a separate file to keep the version numbers that we are depending on in one place. Is it a best practice in dotnet apps as well? Can we move the dependencies version number into one place. I know it’s complitaced because of the different usage and file formats and all, just it would be nice to have dotner version, dotnet path, helix version, dotnet arcade version, xunit version, etc all in one place possibly grouped by what is using them, build, test, unit test, etc. Do es it makes sense?
I have no idea how hard it would be to do it? Would it worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, the yml files are the very first files run on the CI so build-time variables are not available yet, and for local builds these yml files are not run at all. I do not think there is a great place for them all.
[3/6/2019]
A brief overview of the changes required here:
additional changes along the way:
fix folder structure of System.Windows.Forms.Design.Tests to be have UnitTests in its path; accompanying sln changemoved to 224 small fixes: include moq in Sys.Win.Forms.Design.Editors; move Sys.Win.Forms.Design.Tests to UnitTests subdir #537add Moq reference to System.Windows.Forms.Design.Editors.csproj (it was missing)moved to 224 small fixes: include moq in Sys.Win.Forms.Design.Editors; move Sys.Win.Forms.Design.Tests to UnitTests subdir #537