Merge pull request microsoft#424 from chrisd8088/func-test-extra-ci

We simplify the options for the functional test program, align them between 
macOS and Windows (including in CI), update the relevant documentation,
and fix a failing test so it runs and passes on all platforms.

In the functional test program we remove the ExtraCoverage category and
--extra-only and --windows-only options.  We align the Mac and Windows
RunFunctionalTests scripts to not use --full-suite by default in either case,
but we do pass that option in Azure Pipelines CI jobs on PRs in case it becomes
meaningful in the future.  We also remove some leftover GitStatusCache setup.

We update the AuthoringTests.md documentation to reflect these changes
and others.

To resolve the test failure we simply check for the continued presence of the
locked bad pack file after the first fetch attempt instead of parsing for a specific
text string, and mark the test as WindowsOnly as it does not apply to macOS.

.azure-pipelines/templates/osx/functional-test.yml

@@ -40,7 +40,7 @@ steps:
     - bash: watchman log-level debug
       displayName: Set watchman log level to debug
 
-  - bash: $(Build.BinariesDirectory)/ft/src/Scripts/Mac/RunFunctionalTests.sh $(configuration) --test-scalar-on-path --trace2-output=$(Build.ArtifactStagingDirectory)/logs/trace2-event.log
+  - bash: $(Build.BinariesDirectory)/ft/src/Scripts/Mac/RunFunctionalTests.sh $(configuration) --full-suite --test-scalar-on-path --trace2-output=$(Build.ArtifactStagingDirectory)/logs/trace2-event.log
     displayName: Run functional tests
 
   - ${{ if eq(parameters.useWatchman, 'true') }}:

.azure-pipelines/templates/win/functional-test.yml

@@ -32,7 +32,7 @@ steps:
   - script: git config --global credential.interactive never
     displayName: Disable interactive auth
 
-  - script: $(Build.BinariesDirectory)\ft\src\Scripts\RunFunctionalTests.bat $(configuration) --test-scalar-on-path "--trace2-output=$(Build.ArtifactStagingDirectory)\logs\trace2-event.log"
+  - script: $(Build.BinariesDirectory)\ft\src\Scripts\RunFunctionalTests.bat $(configuration) --full-suite --test-scalar-on-path "--trace2-output=$(Build.ArtifactStagingDirectory)\logs\trace2-event.log"
     displayName: Run functional tests
 
   - ${{ if eq(parameters.useWatchman, 'true') }}:

AuthoringTests.md

@@ -5,19 +5,8 @@
 #### Runnable functional test projects
 
 - `Scalar.FunctionalTests`
-- `Scalar.FunctionalTests.Windows`
 
-`Scalar.FunctionalTests` is a .NET Core project and contains all cross-platform functional tests.  `Scalar.FunctionalTests.Windows`, contains functional tests that require Windows. Additionally, `Scalar.FunctionalTests.Windows` includes all the `Scalar.FunctionalTests` allowing it to run both cross-platform and Windows-specific functional tests.
-
-#### Other functional test projects
-
-*Scalar.NativeTests*
-
-`Scalar.NativeTests` contains tests written in C++ that use the Windows API directly.  These tests are called from the managed tests (see above) using PInvoke.
-
-*Scalar.FunctionalTests.LockHolder*
-
-The `LockHolder` is a small program that allows the functional tests to request and release the `ScalarLock`.  `LockHolder` is useful for simulating different timing/race conditions.
+`Scalar.FunctionalTests` is a .NET Core project and contains all cross-platform functional tests.
 
 ## Running the Functional Tests
 
@@ -34,19 +23,13 @@ The functional tests are built on NUnit 3, which is available as a set of NuGet
 2. Run the Scalar installer that was built in step 2.  This will ensure that Scalar will be able to find the correct version of the pre/post-command hooks. The installer will be placed in `BuildOutput\Scalar.Installer.Windows\bin\x64\<Debug or Release>`
 3. Run the tests **with elevation**.  Elevation is required because the functional tests create and delete a test service.
 
-   **Option 1:** Run the `Scalar.FunctionalTests.Windows` project from inside Visual Studio launched as Administrator.
+   **Option 1:** Run the `Scalar.FunctionalTests` project from inside Visual Studio launched as Administrator.
 
    **Option 2:** Run `Scripts\RunFunctionalTests.bat` from CMD launched as Administrator.
 
 #### Selecting Which Tests are Run
 
-By default, the functional tests run a subset of tests as a quick smoke test for developers.  There are three mutually exclusive arguments that can be passed to the functional tests to change this behavior:
-
-- `--full-suite`: Run all configurations of all functional tests
-- `--extra-only`: Run only those tests marked as "ExtraCoverage" (i.e. the tests that are not run by default)
-- `--windows-only`: Run only the tests marked as being Windows specific
-
-**NOTE** `Scripts\RunFunctionalTests.bat` already uses some of these arguments.  If you run the tests using `RunFunctionalTests.bat` consider locally modifying the script rather than passing these flags as arguments to the script.
+By default, the functional tests run on a single configuration.  Passing the `--full-suite` option runs all tests on all configurations.
 
 ### Mac
 
@@ -79,34 +62,30 @@ Note that the test name must include the class and namespace and that `Debug` or
 
 Windows (Script):
 
-`Scripts\RunFunctionalTests.bat Debug --test=Scalar.FunctionalTests.Tests.EnlistmentPerFixture.GitFilesTests.CreateFileTest`
+`Scripts\RunFunctionalTests.bat Debug --test=Scalar.FunctionalTests.Tests.EnlistmentPerFixture.CloneTests.CloneToPathWithSpaces
 
 Windows (Visual Studio):
 
-1. Set `Scalar.FunctionalTests.Windows` as StartUp project
-2. Project Properties->Debug->Start options->Command line arguments (all on a single line): `--test=Scalar.FunctionalTests.Tests.EnlistmentPerFixture.GitFilesTests.CreateFileTest`
+1. Set `Scalar.FunctionalTests` as StartUp project
+2. Project Properties->Debug->Start options->Command line arguments (all on a single line): `--test=Scalar.FunctionalTests.Tests.EnlistmentPerFixture.CloneTests.CloneToPathWithSpaces
 
 Mac:
 
-`Scripts/Mac/RunFunctionalTests.sh Debug --test=Scalar.FunctionalTests.Tests.EnlistmentPerFixture.GitFilesTests.CreateFileTest`
+`Scripts/Mac/RunFunctionalTests.sh Debug --test=Scalar.FunctionalTests.Tests.EnlistmentPerFixture.CloneTests.CloneToPathWithSpaces`
 
 ## How to Write a Functional Test
 
 Each piece of functionality that we add to Scalar should have corresponding functional tests that clone a repo and use existing tools and filesystem APIs to interact with the virtual repo.
 
 Since these are functional tests that can potentially modify the state of files on disk, you need to be careful to make sure each test can run in a clean 
-environment.  There are three base classes that you can derive from when writing your tests.  It's also important to put your new class into the same namespace
+environment.  There are two base classes that you can derive from when writing your tests.  It's also important to put your new class into the same namespace
 as the base class, because NUnit treats namespaces like test suites, and we have logic that keys off that for deciding when to create enlistments.
 
-1. `TestsWithLongRunningEnlistment`
-
-    Before any test in this namespace is executed, we create a single enlistment.  We then run all tests in this namespace that derive from this base class.  Only put tests in here that are purely readonly and will leave the repo in a good state for future tests.
-
-2. `TestsWithEnlistmentPerFixture`
+1. `TestsWithEnlistmentPerFixture`
 
     For any test fixture (a fixture is the same as a class in NUnit) that derives from this class, we create an enlistment before running any of the tests in the fixture, and then we delete the enlistment after all tests are done (but before any other fixture runs).  If you need to write a sequence of tests that manipulate the same repo, this is the right base class.
 
-3. `TestsWithEnlistmentPerTestCase`
+2. `TestsWithEnlistmentPerTestCase`
 
    Derive from this class if you need a new enlistment created for each test case.  This is the most reliable, but also most expensive option.
 

Scalar.FunctionalTests/Categories.cs

@@ -2,7 +2,6 @@ namespace Scalar.FunctionalTests
 {
     public static class Categories
     {
-        public const string ExtraCoverage = "ExtraCoverage";
         public const string GitCommands = "GitCommands";
 
         public const string WindowsOnly = "WindowsOnly";

Scalar.FunctionalTests/Program.cs

@@ -69,16 +69,6 @@ public static void Main(string[] args)
             }
             else
             {
-                if (runner.HasCustomArg("--extra-only"))
-                {
-                    Console.WriteLine("Running only the tests marked as ExtraCoverage");
-                    includeCategories.Add(Categories.ExtraCoverage);
-                }
-                else
-                {
-                    excludeCategories.Add(Categories.ExtraCoverage);
-                }
-
                 ScalarTestConfig.FileSystemRunners = FileSystemRunners.FileSystemRunner.DefaultRunners;
             }
 
@@ -89,15 +79,6 @@ public static void Main(string[] args)
             }
             else
             {
-                if (runner.HasCustomArg("--windows-only"))
-                {
-                    includeCategories.Add(Categories.WindowsOnly);
-
-                    // RunTests unions all includeCategories.  Remove ExtraCoverage to
-                    // ensure that we only run tests flagged as WindowsOnly
-                    includeCategories.Remove(Categories.ExtraCoverage);
-                }
-
                 excludeCategories.Add(Categories.MacOnly);
             }
 
@@ -135,18 +116,6 @@ private static void RunBeforeAnyTests()
             if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
             {
                 ScalarServiceProcess.InstallService();
-
-                string statusCacheVersionTokenPath = Path.Combine(
-                    Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles, Environment.SpecialFolderOption.Create),
-                    "Scalar",
-                    "ProgramData",
-                    "Scalar.Service",
-                    "EnableGitStatusCacheToken.dat");
-
-                if (!File.Exists(statusCacheVersionTokenPath))
-                {
-                    File.WriteAllText(statusCacheVersionTokenPath, string.Empty);
-                }
             }
         }
     }

Scalar.FunctionalTests/Tests/EnlistmentPerFixture/CacheServerTests.cs

@@ -5,7 +5,6 @@
 namespace Scalar.FunctionalTests.Tests.EnlistmentPerFixture
 {
     [TestFixture]
-    [Category(Categories.ExtraCoverage)]
     public class CacheServerTests : TestsWithEnlistmentPerFixture
     {
         private const string CustomUrl = "https://myCache";

Scalar.FunctionalTests/Tests/EnlistmentPerFixture/FetchStepWithoutSharedCacheTests.cs

@@ -8,7 +8,6 @@
 namespace Scalar.FunctionalTests.Tests.EnlistmentPerFixture
 {
     [TestFixture]
-    [Category(Categories.ExtraCoverage)]
     public class FetchStepWithoutSharedCacheTests : TestsWithEnlistmentPerFixture
     {
         private const string PrefetchPackPrefix = "prefetch";
@@ -101,8 +100,9 @@ public void FetchStepCleansUpBadPrefetchPack()
             this.TempPackRoot.ShouldBeADirectory(this.fileSystem).WithNoItems();
         }
 
+        // WindowsOnly because the test depends on Windows-specific file sharing behavior
         [TestCase, Order(4)]
-        [Category(Categories.MacTODO.TestNeedsToLockFile)]
+        [Category(Categories.WindowsOnly)]
         public void FetchStepFailsWhenItCannotRemoveABadPrefetchPack()
         {
             this.Enlistment.Unregister();
@@ -119,10 +119,11 @@ public void FetchStepFailsWhenItCannotRemoveABadPrefetchPack()
             // Open a handle to the bad pack that will prevent fetch-commits-and-trees from being able to delete it
             using (FileStream stream = new FileStream(badPackPath, FileMode.Open, FileAccess.Read, FileShare.None))
             {
-                string output = this.Enlistment.RunVerb("fetch", failOnError: false);
-                output.ShouldContain($"Unable to delete {badPackPath}");
+                this.Enlistment.RunVerb("fetch", failOnError: false);
             }
 
+            badPackPath.ShouldBeAFile(this.fileSystem).WithContents(badContents);
+
             // After handle is closed fetching commits and trees should succeed
             this.Enlistment.RunVerb("fetch");
 

Scalar.FunctionalTests/Tests/EnlistmentPerFixture/ScalarUpgradeReminderTests.cs

@@ -12,7 +12,6 @@ namespace Scalar.FunctionalTests.Tests.EnlistmentPerFixture
 {
     [TestFixture]
     [NonParallelizable]
-    [Category(Categories.ExtraCoverage)]
     [Category(Categories.WindowsOnly)]
     [Category(Categories.NeedsUpdatesForNonVirtualizedMode)]
     public class UpgradeReminderTests : TestsWithEnlistmentPerFixture

Scalar.FunctionalTests/Tests/MultiEnlistmentTests/ConfigVerbTests.cs

@@ -7,7 +7,6 @@
 namespace Scalar.FunctionalTests.Tests.MultiEnlistmentTests
 {
     [TestFixture]
-    [Category(Categories.ExtraCoverage)]
     [Category(Categories.NeedsUpdatesForNonVirtualizedMode)]
     public class ConfigVerbTests : TestsWithMultiEnlistment
     {

Scripts/Mac/RunFunctionalTests.sh

@@ -18,4 +18,4 @@ shift
 chmod +x $TESTS_EXEC
 
 # Run the tests!
-$TESTS_EXEC --full-suite "$@"
+$TESTS_EXEC "$@"