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

Helix proof of concept for unit tests #224

Merged
merged 95 commits into from
Mar 8, 2019
Merged

Helix proof of concept for unit tests #224

merged 95 commits into from
Mar 8, 2019

Conversation

AdamYoblick
Copy link
Member

@AdamYoblick AdamYoblick commented Dec 10, 2018

[3/6/2019]

A brief overview of the changes required here:

  • Add new step to send unit tests to helix (for this, I have abstracted away to a new file: eng/ci-helix-test.yml); accompanying work in ci.yml
  • Add dependency on Microsoft.DotNet.Helix.sdk from Arcade
  • Add dependencies to XUnit, XUnitAssert, XUnitRunnerConsole, XUnitRunnerVisualStudio, and XUnitExtensibilityExecution (all with the same version number)

additional changes along the way:

@AdamYoblick AdamYoblick requested a review from a team as a code owner December 10, 2018 21:53
@zsd4yr
Copy link
Contributor

zsd4yr commented Mar 7, 2019

Big shoutout to @AdamYoblick for his work on this earlier and also @alexperovich for his assistance as well as support on the Arcade bits

@zsd4yr
Copy link
Contributor

zsd4yr commented Mar 7, 2019

Waiting on #537

@zsd4yr zsd4yr requested a review from alexperovich March 7, 2019 19:50
@zsd4yr
Copy link
Contributor

zsd4yr commented Mar 7, 2019

@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 ✔️ 😉

eng/ci-helix-test.yml Outdated Show resolved Hide resolved
}

ProcessStartInfo startInfo = new ProcessStartInfo();
startInfo.FileName = Path.Combine(BinPath(), byPathFromBinToExe.Trim('\\'));
startInfo.EnvironmentVariables["DOTNET_ROOT"] = dotnetPath;
startInfo.EnvironmentVariables["DOTNET_ROOT"] = dotnetPath; // required
Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

@Tanya-Solyanik Tanya-Solyanik Mar 8, 2019

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?

Copy link
Member

@sharwell sharwell left a 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>
Copy link
Contributor

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?

Copy link
Contributor

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.

@zsd4yr zsd4yr merged commit 86bbead into master Mar 8, 2019
@zsd4yr zsd4yr deleted the dev/advolker/helix branch March 8, 2019 17:57
zsd4yr added a commit that referenced this pull request Mar 8, 2019
zsd4yr added a commit that referenced this pull request Mar 8, 2019
zsd4yr added a commit that referenced this pull request Mar 8, 2019
zsd4yr added a commit that referenced this pull request Mar 8, 2019
zsd4yr added a commit that referenced this pull request Mar 8, 2019
zsd4yr added a commit that referenced this pull request Mar 11, 2019
* add creator, set to empty in internal, comment edits

* helix token is secret AzDO pipeline variable

* add xunit assert version

* helix access token

* helix source, differ from internal to public
@ghost ghost locked as resolved and limited conversation to collaborators Feb 8, 2022
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.

7 participants