Improves unit test results stability across runs #107
Conversation
As of Visual Studio 2019, you must now add a Test Adapter NuGet package to unit test projects so that the unit tests can be discovered and run. This commit adds the NUnit 2 test adapter NuGet package. Also, the unit tests project was updated to include the ProjectTypeGuids that let Visual Studio know the project is a test project and provide the Run Tests option in the context menu of the project. As an NCrunch user, NCrunch can greatly increase stability of unit tests across test runs. This commit contains some changes that help improve the stability of test results across test runs. To improve stability across test runs, the post-build event on the OctoPack.Tasks project was replaced with a custom target in the OctoPack.Tests project that executes as the last target that is run as part of a build of that project. In addition, the OctoPack.Tests project did not directly reference the OctoPack.Tests.SampleGitVersionAssembly or OctoPack.Tests.NonGitVersionAssembly projects, but instead, used complicated assembly locating logic which is fragile and simply not usable when executing the AssemblyExtensionsTasks tests by NCrunch. Instead, directly reference those assemblies. When configuring NCrunch with <CopyReferenceAssembliesToWorkspace>true</CopyReferenceAssembliesToWorkspace>, "It just Works". (NCrunch also needs to be configured to include additional files, such as the ..\..\build\**.* and ..\Samples\**.* files.) Lastly, I also added the OctoPack.Tests.SampleExeDependency project to the solution. It was there--just never added to the solution. With these changes in place, the tests are running locally and I can safely make changes to the project.
This commit removes files generated during the build of projects in the Samples solution from the repository and adds a .gitignore file to the Samples directory to further ensure these files do not get accidentally re-added to the repository. These files, if present, can cause the unit tests to fail since the unit tests currently do not do a great job of cleaning up after themselves. This change highlighted another assumption made in one of the tests, that one of the sample project files was already copied to *.csproj.teamcity. So this test was updated to explicitly copy the required file, further improving test stability in different environments across test runs.
There were no tests that tested the installation/uninstallation of OctoPack. So I added some. Basically, the tests ensure that when OctoPack is installed/uninstalled that the project file into which it's being installed/uninstalled is modified in an expected manner.
| @@ -1,10 +1,12 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <Import Project="..\packages\NUnitTestAdapter.2.3.0\build\NUnitTestAdapter.props" Condition="Exists('..\packages\NUnitTestAdapter.2.3.0\build\NUnitTestAdapter.props')" /> | |||
There was a problem hiding this comment.
I couldn't run the tests via build.cmd or via my IDE with this test adapter.
There was a problem hiding this comment.
So the Test Adapter is only used when running the tests from within Visual Studio.
There was a problem hiding this comment.
I amend my former comment. The test adapter is used when running the tests using vstest.console.exe (which internally, Visual Studio uses to run the tests, too).
Now, when it comes to Cake, I don't know how they implemented their NUnit function. Perhaps it uses the Nunit console runner?
tothegills
left a comment
tothegills
left a comment
There was a problem hiding this comment.
Two test failures associated with this change:
OctoPack.Tests.Integration.SampleSolutionBuildFixture.ShouldIncludeTypeScriptOutputs
These files were expected to be in the package:
@"Scripts\MyTypedScript.js",
@"Scripts\MyTypedScript.UI.js"
at NUnit.Framework.Assert.Fail(String message, Object[] args)
at OctoPack.Tests.Integration.PackageArchiveReaderExtensions.AssertContents(PackageArchiveReader package, String[] files) in O:\repo\OctoPack\source\OctoPack.Tests\Util\PackageArchiveReaderExtensions.cs:line 73
at OctoPack.Tests.Integration.SampleSolutionBuildFixture.<>c.<ShouldIncludeTypeScriptOutputs>b__16_0(PackageArchiveReader pkg) in O:\repo\OctoPack\source\OctoPack.Tests\Integration\SampleSolutionBuildFixture.cs:line 240
at OctoPack.Tests.Integration.BuildFixture.AssertPackage(String packageFilePath, Action`1 packageAssertions) in O:\repo\OctoPack\source\OctoPack.Tests\Integration\BuildFixture.cs:line 89
at OctoPack.Tests.Integration.SampleSolutionBuildFixture.ShouldIncludeTypeScriptOutputs() in O:\repo\OctoPack\source\OctoPack.Tests\Integration\SampleSolutionBuildFixture.cs:line 239
OctoPack.Tests.Integration.SampleSolutionBuildFixture.ShouldIncludeTypeScriptSourcesWhenSpecified
These files were expected to be in the package:
@"Scripts\MyTypedScript.js",
@"Scripts\MyTypedScript.UI.js"
at NUnit.Framework.Assert.Fail(String message, Object[] args)
at OctoPack.Tests.Integration.PackageArchiveReaderExtensions.AssertContents(PackageArchiveReader package, String[] files) in O:\repo\OctoPack\source\OctoPack.Tests\Util\PackageArchiveReaderExtensions.cs:line 73
at OctoPack.Tests.Integration.SampleSolutionBuildFixture.<>c.<ShouldIncludeTypeScriptSourcesWhenSpecified>b__15_0(PackageArchiveReader pkg) in O:\repo\OctoPack\source\OctoPack.Tests\Integration\SampleSolutionBuildFixture.cs:line 220
at OctoPack.Tests.Integration.BuildFixture.AssertPackage(String packageFilePath, Action`1 packageAssertions) in O:\repo\OctoPack\source\OctoPack.Tests\Integration\BuildFixture.cs:line 89
at OctoPack.Tests.Integration.SampleSolutionBuildFixture.ShouldIncludeTypeScriptSourcesWhenSpecified() in O:\repo\OctoPack\source\OctoPack.Tests\Integration\SampleSolutionBuildFixture.cs:line 219
|
I ran the tests three different ways: Visual Studio IDE (the standard test runner), NCrunch, and via the Cake script. In fact, I ran the Cake script before pushing up this commit to make this PR. In fact, I ran the Cake script before pushing up each of the 3 PRs I created! 😄 So, I wonder what's different about your environment, or how you're running the tests? Did you actually do a build first? Sorry, have to ask, because the build contains a TypeScript compiler that compiles the TypeScript files to generate those JavaScript files mentioned in the error message. However, having just written that, it occurs to me that the unit test should be automatically building that project, causing those JavaScript files to be generated, and then which ought to be included in the OctoPack package. |
|
Hi @fourpastmidnight, shipping your other 2 PRs as 3.6.5. This PR makes me the most nervous, not entirely convinced the changes are worth it considering how risk-averse we are for OctoPack. |
|
@tothegills Will a NuGet package for 3.65 soon be published? It's not showing up in the releases tab, which is why I'm asking. |
|
@tothegills Never mind—I see it is up on NuGet.org. Thanks! I'm going to pull down this branch again and see if I encounter errors running the tests. |
|
We have flagged this as an issue with product, but at this point we do not have capacity to assign an engineer to resolve the issue. We'll keep the issue open, but unfortunately we can not provide any guidance as to when it will be resolved. |
3 participants
This pull request makes some changes to the unit tests of this project to improve the stability of test results across test runs.
In addition, OctoPack installation/uninstallation integration tests were added to ensure OctoPack is modifying a C# project file in an expected manner.
I also added entries to .gitignore to ignore NCrunch specific files, since I'm an NCrunch user and I was asked to ensure that these files are not committed to the repository.