Implement Package Tests for NUnit Console Runner #896
Conversation
|
@ChrisMaddock @nunit/engine-team |
|
Hi Charlie, I like the concept. I also like how much is extracted into separate classes, meaning we could quite easily port the testing into an NUnit assembly in future, if we did want to. Do you want to carry on and tidy up, and then I'll have a proper review of the detail? wrt what's in and what's not, I largely agree with your list. v2 execution - is this something you already have written? It might be nice to include it as a test of running an IFrameworkDriver. We may not want to package the nunit v2 driver with the v4 engine, but we will still support IFrameworkDriver's, and I think the v2 driver is the only one I know of that currently exists! That could be a later addition however, if you wanted to get this wrapped up. |
|
So long as you like the direction, I can go ahead and complete this. Where I have questions or options, I'll add review comments myself. Yes, V2 execution is already written and mostly in place. The biggest thing needed to add a test case is to have a V2 mock assembly to run. Note that while I can install any extension using the script, I have not succeeded in doing it for the .NET Core Runner. I've run into the same thing working on the extensions themselves. At some point, we'll need to deal with this. The other pending question is what level you want the tests to run at. For the number we have, there's no problem running all of them all the time, but as more might be added, we may want to use all three levels. I'll note that with a comment when I update the PR. |
|
I cleaned up and updated the description of what's included and what's not. This is ready for review now. |
|
@ChrisMaddock @nunit/engine-team Hey guys, can we get a review of this? |
| @@ -24,5 +24,5 @@ | |||
| using System.Reflection; | |||
|
|
|||
| [assembly: AssemblyProduct("NUnit Engine")] | |||
| [assembly: AssemblyVersion("3.13.0")] | |||
| [assembly: AssemblyVersion("3.13.0.0")] | |||
There was a problem hiding this comment.
Is this change intended? (same comment for the ConsoleVersion.cs)
There was a problem hiding this comment.
Generally, assembly versions are supposed to have four components, while packages using semantic versioning have 3. This commit back in 2018 changed it for NUnit-console. I'd like to hear from @ChrisMaddock about how he wants to handle this, but it seems to me incorrect not to specify the last component here.
| build-scripts\constants.cake = build-scripts\constants.cake | ||
| build-scripts\package-checks.cake = build-scripts\package-checks.cake | ||
| build-scripts\package-tests.cake = build-scripts\package-tests.cake | ||
| build-scripts\test-results.cake = build-scripts\test-results.cake |
There was a problem hiding this comment.
| build-scripts\test-results.cake = build-scripts\test-results.cake | |
| build-scripts\test-results.cake = build-scripts\test-results.cake | |
| build-scripts\testing.cake = build-scripts\testing.cake |
There was a problem hiding this comment.
@mikkelbu I don't see what the change is here.
There was a problem hiding this comment.
I also included the line build-scripts\testing.cake = build-scripts\testing.cake
| //PackageTests.Add(new PackageTest(2, "Run mock-assembly.dll built for NUnit V2", | ||
| // "v2-tests/mock-assembly.dll", | ||
| // new ExpectedResult("Failed") | ||
| // { | ||
| // Total = 28, | ||
| // Passed = 18, | ||
| // Failed = 5, | ||
| // Warnings = 0, | ||
| // Inconclusive = 1, | ||
| // Skipped = 4 | ||
| // }, | ||
| // NUnitV2Driver)); |
There was a problem hiding this comment.
As far as I can tell this is the only place where the new mock assembly is mentioned?
There was a problem hiding this comment.
Yes... the goal is to activate this test in a follow-up PR.
There was a problem hiding this comment.
I think this should be uncommented and in use, right?
| @@ -0,0 +1,272 @@ | |||
| // *********************************************************************** | |||
| // Copyright (c) Charlie Poole and TestCentric GUI contributors. | |||
There was a problem hiding this comment.
Should this be changed? I found a similar header in AgentProcess.cs and AgentProcessTests.cs
There was a problem hiding this comment.
Will change these in next commit.
|
@mikkelbu Re the unused V2 Test assembly... I added it as well as the commented-out test but I can't get the V2 test to run without significant changes to the structure of the package itself. After discussion with Chris I commented out that test but left the assembly there because it took me a few hours to get it building correctly with the NUnit 2.6.4 reference! I'm anticipating getting it working in another PR so I left it there. I'll clean up some of the other issues you noted in that assembly, which carried over from the GUI version. |
Co-authored-by: Mikkel Nylander Bundgaard <mikkelbu@users.noreply.github.com>
| //// URLs for uploading packages | ||
| //private const string MYGET_PUSH_URL = "https://www.myget.org/F/nunit/api/v2"; | ||
| //private const string NUGET_PUSH_URL = "https://api.nuget.org/v3/index.json"; | ||
| //private const string CHOCO_PUSH_URL = "https://push.chocolatey.org/"; | ||
|
|
||
| //// Environment Variable names holding API keys | ||
| //private const string MYGET_API_KEY = "MYGET_API_KEY"; | ||
| //private const string NUGET_API_KEY = "NUGET_API_KEY"; | ||
| //private const string CHOCO_API_KEY = "CHOCO_API_KEY"; | ||
|
|
||
| //// Environment Variable names holding GitHub identity of user | ||
| //private const string GITHUB_OWNER = "TestCentric"; | ||
| //private const string GITHUB_REPO = "testcentric-gui"; | ||
| //// Access token is used by GitReleaseManager | ||
| //private const string GITHUB_ACCESS_TOKEN = "GITHUB_ACCESS_TOKEN"; | ||
|
|
||
| //// Pre-release labels that we publish | ||
| //private static readonly string[] LABELS_WE_PUBLISH_ON_MYGET = { "dev", "pre" }; | ||
| //private static readonly string[] LABELS_WE_PUBLISH_ON_NUGET = { "alpha", "beta", "rc" }; | ||
| //private static readonly string[] LABELS_WE_PUBLISH_ON_CHOCOLATEY = { "alpha", "beta", "rc" }; |
There was a problem hiding this comment.
Will this be used in the (near) future?
There was a problem hiding this comment.
I don't know. The original files were copied from the GUI. If we want to go all the way to automatic publication, then I'd like to leave them here, just so as not to have to repeat the work. If we don't, then I'll delete them. I have the code, so I could take the next step in a matter of days, but I'm waiting for direction from @ChrisMaddock.
There was a problem hiding this comment.
Automatic publication would be great, but there's over 3,000 lines of change in this PR already - I'd really appreciate if we could tackle that in a separate issue!
| bool isOK = | ||
| CheckNuGetPackage( | ||
| "NUnit.Console", | ||
| HasFile("LICENSE.txt")) & |
There was a problem hiding this comment.
Just a personal opinion, but I think I would prefer separate variabels for each package and then check them all in the if below (I overlooked all the & at the first look at the file). Or perhaps just store all the results from all the CheckNuGetPackages in an array/list, so we have a sequence of booleans, and then ensure that all the results are true.
There was a problem hiding this comment.
In fact, that's how my original code is structured. Each package type has a set of tests and a set of checks However, I didn't do that for nunit-console because only all packages have checks but only some have tests. I suppose the tests could be empty in that case. I can do another pass at this after my other stuff is merged.
| //public void PatchAssemblyInfo(string sourceFile, string assemblyVersion = null) | ||
| //{ | ||
| // ReplaceFileContents(sourceFile, source => | ||
| // { | ||
| // source = ReplaceAttributeString(source, "AssemblyVersion", assemblyVersion ?? _parameters.AssemblyVersion); | ||
|
|
||
| // source = ReplaceAttributeString(source, "AssemblyFileVersion", _parameters.AssemblyFileVersion); | ||
|
|
||
| // source = ReplaceAttributeString(source, "AssemblyInformationalVersion", _parameters.AssemblyInformationalVersion); | ||
|
|
||
| // return source; | ||
| // }); | ||
|
|
||
| // string ReplaceAttributeString(string source, string attributeName, string value) | ||
| // { | ||
| // var matches = Regex.Matches(source, $@"\[assembly: {Regex.Escape(attributeName)}\(""(?<value>[^""]+)""\)\]"); | ||
| // if (matches.Count != 1) throw new InvalidOperationException($"Expected exactly one line similar to:\r\n[assembly: {attributeName}(\"1.2.3-optional\")]"); | ||
|
|
||
| // var group = matches[0].Groups["value"]; | ||
| // return source.Substring(0, group.Index) + value + source.Substring(group.Index + group.Length); | ||
| // } | ||
|
|
||
| // void ReplaceFileContents(string filePath, Func<string, string> update) | ||
| // { | ||
| // using (var file = new FileStream(filePath, FileMode.Open, FileAccess.ReadWrite, FileShare.None)) | ||
| // { | ||
| // string source; | ||
| // using (var reader = new StreamReader(file, new UTF8Encoding(false), true, 4096, leaveOpen: true)) | ||
| // source = reader.ReadToEnd(); | ||
|
|
||
| // var newSource = update.Invoke(source); | ||
| // if (newSource == source) return; | ||
|
|
||
| // file.Seek(0, SeekOrigin.Begin); | ||
| // using (var writer = new StreamWriter(file, new UTF8Encoding(false), 4096, leaveOpen: true)) | ||
| // writer.Write(newSource); | ||
| // file.SetLength(file.Position); | ||
| // } | ||
| // } | ||
| //} |
There was a problem hiding this comment.
This is almost the same as below (difference is that this also sets AssemblyFileVersion and that it does not check whether the value is specified)
| public string BranchName { get; } | ||
| public bool IsReleaseBranch { get; } | ||
|
|
||
| public string PackageVersion { get; } |
| @@ -0,0 +1,312 @@ | |||
| #load "./constants.cake" | |||
There was a problem hiding this comment.
General comments for this file:
- There are a lot of commented code
- Mix of tabs and spaces for indention
There was a problem hiding this comment.
I agree with Mikkel here - would you mind removing all the commented lines from these files, and we can port them in future, if we chose too? There's more commented lines than uncommented in some of these files - I am finding it harder to follow and review because of these.
There was a problem hiding this comment.
I'll fix the tabs and remove any commented-out code that isn't related to future functionality. Once @ChrisMaddock provides direction on future functionality, I'll either implement it or delete it.
There was a problem hiding this comment.
Responded below - this PR is pretty huge from a reviewing perspective - would appreciate if we could keep future functionality for a separate PR for now! 😄
|
|
||
| if (validationErrors.Count > 0) | ||
| { | ||
| DumpSettings(); |
There was a problem hiding this comment.
Currently, this is dead code, as we don't add anything to validationErrors
There was a problem hiding this comment.
Hi Charlie - have spent a couple of hours going through this tonight - the overall aim here is great, but there's vast amounts of new code, and I'm finding it pretty difficult to follow through! I presume we're past the point of breaking this into multiple PR's now? Would you be able to give something of an overview (in line comments) in terms of what's new code and logic, and what's just moving around?
I will say reviewing this is definitely making me miss VS from a code comprehension point of view - that's unfortunately one thing we lose with Cake.
Am I also right in thinking quite a bit of this isn't in use yet, and could perhaps be simplified? Or am I just missing where it's called from? I think simplifying what we can would be really helpful if it is possible - and then build on it as we add in any new functionality.
| @@ -0,0 +1,312 @@ | |||
| #load "./constants.cake" | |||
There was a problem hiding this comment.
I agree with Mikkel here - would you mind removing all the commented lines from these files, and we can port them in future, if we chose too? There's more commented lines than uncommented in some of these files - I am finding it harder to follow and review because of these.
| if (matches.Count != 1) throw new InvalidOperationException($"Expected exactly one line similar to:\r\n[assembly: {attributeName}(\"1.2.3-optional\")]"); | ||
|
|
||
| var group = matches[0].Groups["value"]; | ||
| return source.Substring(0, group.Index) + value + source.Substring(group.Index + group.Length); |
There was a problem hiding this comment.
Would you mind adding some comments to this file? I'm not totally clear what all the regex is doing here. Out of interest, what's this improving over our old versioning methods?
| public string AssemblyFileVersion => PackageVersion; | ||
| public string AssemblyInformationalVersion => PackageVersion; | ||
|
|
||
| //public void PatchAssemblyInfo(string sourceFile, string assemblyVersion = null) |
There was a problem hiding this comment.
I'm not too clear what's been done here sorry. Are both versions of this method necessary? What's been changed here, sorry?
| build-scripts\package-checks.cake = build-scripts\package-checks.cake | ||
| build-scripts\package-tests.cake = build-scripts\package-tests.cake | ||
| build-scripts\test-results.cake = build-scripts\test-results.cake | ||
| build-scripts\versioning.cake = build-scripts\versioning.cake |
There was a problem hiding this comment.
One suggestion on this based on my own experience - could it be confusing to newcomers that the build-scripts directory doesn't contain the main build script? 🙂 Maybe we should move it all to a /build/? Or rename the directory cake-files or something?
| // 0 Do not run - used for temporarily disabling a test | ||
| // 1 Run for all CI tests - that is every time we test packages | ||
| // 2 Run only on PRs, dev builds and when publishing | ||
| // 3 Run only when publishing |
There was a problem hiding this comment.
Is this functionality used? I think at this point it's probably overkill, right? Maybe we could simplify things for now?
| protected override FilePath PackageUnderTest => _parameters.NuGetNetFXPackage; | ||
| protected override string PackageTestDirectory => _parameters.NuGetNetFXTestDirectory; | ||
| protected override string PackageTestBinDirectory => PackageTestDirectory + "tools/"; | ||
| protected override string ExtensionInstallDirectory => _parameters.TestDirectory; |
There was a problem hiding this comment.
Should this be TestDirectory? Or TempDirectory, where the package is being installed? Maybe I'm missing something around how this works. 😕
| Console.WriteLine($" ERROR: Installer returned {rc.ToString()}"); | ||
| else | ||
| { | ||
| // Administrative install doesn't copy these files |
There was a problem hiding this comment.
Out of interest, could you elaborate on what happens here? Does msiexec somehow not unpack all the files?
|
|
||
| private void CheckCounter(string label, int expected, int actual) | ||
| { | ||
| if (expected > 0 && expected != actual) |
There was a problem hiding this comment.
Should this be >=? Or am I misreading?
|
|
||
| // This file contains classes used to interpret the result XML that is | ||
| // produced by test runs of the GUI. | ||
|
|
There was a problem hiding this comment.
Actually what's this file for, sorry? It seems to just be returning a single number - although I'm presuming there's a reason for this, rather than just checking the console exit code?
| protected static readonly string[] NET_CORE_AGENT_FILES = { | ||
| "testcentric-agent.dll", "testcentric-agent.dll.config", "testcentric-agent-x86.dll", "testcentric-agent-x86.dll.config" }; | ||
| protected static readonly string[] GUI_FILES = { | ||
| "testcentric.exe", "testcentric.exe.config", "tc-next.exe", "tc-next.exe.config", "nunit.uiexception.dll", |
There was a problem hiding this comment.
Should this file be included?
|
Couple other things:
|
|
@ChrisMaddock I'm OK with that provided we both have the same idea of what the MVP is!!! |
|
I'm working on some other stuff, so I'll set this aside for a few days and then come back with a smaller PR. |
|
OK.. I'm back to this faster than I thought. I'm starting over but won't remove this PR until the other work is done. @ChrisMaddock @mikkelbu I apologize for my impatience. I had created three more branches, chained to this PR, which I'll now have to do over as well. It was my mistake to create them that way in the first place. That said, this work was out there for almost three weeks without any review. The next ones will be in smaller chunks and I'll avoid changes to the overall architecture of the scripts. I hope that will help folks to get in and review them more quickly. |
|
Thanks Charlie. Happy with changes to the architecture of the scripts if that makes the porting easier - but think it would be easier if the refactoring was separated out into separate PRs? Maybe that would be a good way to move this on. Personally I'd be happy for you to merge any work that's "just refactoring" straight in - and we can just review the new logic? Could help make the most of everyone's time? Leave it with you as to what you think is best. |
|
@ChrisMaddock I'll try that and we can see how it works. |
|
Closing this PR, which has been replaced by #902 |
Fixes #852 and Fixes #892
NOTE: I inadvertently kept working on the issue-852 branch, which should have been a separate PR. However #852 was a very small change, so I'm leaving it here.
At this point, I'm creating the PR so we can discuss whether further work is needed and, if so, whether to keep going on this PR or plan to do it in a separate one.
INCLUDED:
Package content checking for the .NET Core console runner, which was previously excluded.
Restructured our scripts to make use of parameterized setup and tasks. This is a big change but was required in order to port my package testing code from the TestCentric GUI.
Added testing for five console packages: NuGet for .NET FX, NuGet for .NET Core, Chocolatey, Zip and MSi. There are currently four test runs for each .NET FX package and three for the .NET Core package. The tests are not extensive but they verify that the executable can run without crashing and that the top-level result counters match what is expected. The .NET FX tests include running a
.nunitproject file.Added a .NET 4.0 build of mock-assembly so it could be used in one of the .NET FX tests.
Added an NUnit V2 build of mock-assembly for use in the tests, but ran into an issue and commented out that test for now.
NOT INCLUDED:
Tests of .nunit project under .net core - or of any other extensions - since no extension builds exist for that runtime.
No test of NUnit V2 test execution under .NET FX. The test actually works for all builds but the zip. The zip build is able to load all extensions except the V2 framework driver because that extension is needed by the agent, not the engine. It looks like the zip creation needs to be substantially revised so I'm putting that off for another issue.
The package test code has the ability to set different levels of testing but this is not yet used. All tests are run on every build.
There is no summary report of test results. With 4 packages and 3 or four tests each, I was sometimes confused, so I think this may be a useful extension.
No automatic package uploading or release. That's a separate issue.
NOTE: there's currently commented-out code that's used in the GUI version of the scripts. They relate to points (2), (3) and (5). I prefer to leave the code there for when I take up those issues.