simplify and align functional test CI and docs, and fix failing skipped test#424
Merged
chrisd8088 merged 7 commits intomicrosoft:mainfrom Sep 4, 2020
Merged
Conversation
A limited subset of functional tests are marked as belonging to the ExtraCoverage category, which was introduced in the microsoft/VFSForGit#1046 PR as an enhancement over the FullSuiteOnly category from the Azure Repos PR 302803. The intent was to make the default functional test run faster by excluding some less-fragile tests. However, in Scalar, many of these tests are also marked with NeedsUpdatesForNonVirtualizedMode and therefore do not run at all, while the ones which remain have been running in CI on the macOS platform but not Windows, because Scripts/Mac/RunFunctionalTests.sh passes the --full-suite option. In order to simplify the CI and functional test framework, we eliminate the ExtraCoverage category entirely, which ensures all working tests run on all platforms in CI.
Since the number of Windows-only functional tests is quite small, and we generally want to run all tests in CI, the original purpose of the --windows-only option to the functional test suite has less value than in the VFSForGit project. We also do not have an equivalent --mac-only or --linux-only option. Therefore we simply remove this option, which tidies the program logic and ensures all platforms have an equal set of test options.
We make a number of simplifications to the documentation of the functional test suite to reflect changes from VFSForGit, notably the absence of a number of specialized test programs, classes, and options.
In PR microsoft#29 and commit cedeeaa the VFSForGit GitStatusCache implementation was removed from Scalar. However, the functional test suite still checked for and created the EnableGitStatusCacheToken.dat file if it was missing (but only on Windows). Since this setup is no longer relevant, we can remove it now.
In order to align better with the Windows RunFunctionalTests.bat script, on macOS we change the RunFunctionalTests.sh script so it does not always pass the --full-suite option to the functional test program. This has no substantive impact right now, as the --full-suite option just defines a set of FileSystemRunners applicable to the platform in ScalarTestConfig.FileSystemRunners, which is then returned by FileSystemRunner.Runners() and may be used to parameterize tests so they run with each type of runner. However, at present, the only tests parameterized in this way are the EnlistmentPerFixture.GitMoveRenameTests, which are also in the NeedsUpdatesForNonVirtualizedMode category and thus do not run at all.
Although the --full-suite option to the functional test program does not have any effect at present, per the notes in commit 3a7d9a1, we still choose to pass it in our Azure Pipelines CI jobs in case it becomes significant again in the future.
The FetchStepFailsWhenItCannotRemoveABadPrefetchPack() functional test fails at present, as it looks for specific output from the FetchStep maintenance command in the form of an "Unable to delete" message. This string is no longer written to standard output by the command, however, due to the changes in PR microsoft#295 which introduced a standard progress meter. Instead, that string is only written using this.Context.Tracer.RelatedWarning() in the PerformMaintenance() method of Scalar.Common.Maintenance.FetchStep, and thus appears only in the maintenance log files. The breakage of the test was not noticed because it belongs to the ExtraCoverage category, and therefore has not been running as part of the CI suite on Windows because the RunFunctionalTests.bat script does not supply the --full-suite option. 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. The test is also Windows-specific because it depends on an open file handle with a FileShare.None attribute blocking attempts by other processes to delete the file, and this only works in C# on the Windows platform. On Unix platforms, the C# core libraries implement file sharing in an advisory manner only; see: https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88 We therefore mark this test as WindowsOnly, since it effectively does not apply to other platforms. Note that the previous category assigned to the test, MacTODO.TestNeedsToLockFile, dates from VFSForGit and has been migrated to just this one test as a result of changes in PR microsoft#170. However, the meaning of that category varies, as the one other functional test in the category uses Scalar's FileBasedLock and not C# FileShare settings, so we clarify the situation by just marking this WindowsOnly.
06db5d4 to
f152628
Compare
derrickstolee
approved these changes
Sep 4, 2020
Contributor
derrickstolee
left a comment
There was a problem hiding this comment.
I looked at each commit. Great job splitting into small chunks. Thanks for re-adding test coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to 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.
It may be easiest to review commit-by-commit, but I've also tried to put all of the relevant background into this description as well.
The CI jobs run
Scripts\RunFunctionalTests.bat --test-scalar-on-pathon Windows andScripts/Mac/RunFunctionalTests.sh --test-scalar-on-pathon macOS. However, these two scripts are different in that the Mac one adds the--full-suiteoption while the Windows one does not.One consequence of this is that about 7 additional tests run on macOS than Windows, in the Azure Pipelines CI jobs; for example in this recent build:
Specifically these tests are from the
CacheServerTestsandFetchStepWithoutSharedCacheTestsclasses.One of the latter
FetchStepWithoutSharedCacheTestscurrently fails on Windows (the only platform on which it executed, due to its categorization asMacTODO.TestNeedsToLockFile); see for example this Azure Pipeline run in which theExtraCoveragetests were included on Windows. The failure may have been introduced with the refactoring of theRunVerblogic in PR #295, and not noticed because the CI framework happens to skip this Windows-exclusive job on Windows.First, we remove the
ExtraCoveragecategory and--extra-onlyfunctional test program option, as well as the--windows-onlyoption.The
ExtraCoveragecategory only includes a small number of tests now, and many of those are markedNeedsUpdatesForNonVirtualizedModeso they don't execute at all. Originally introduced in microsoft/VFSForGit#1046 as an enhancement over theFullSuiteOnlycategory from the Azure Repos PR 302803, with the intent of speeding developer test runs. Given the small remaining set of working tests in the category, it seems simplest to just remove it.For similar reasons, we remove the
--windows-onlyoption (while keeping theWindowsOnlycategory). We could add matching--mac-onlyand--linux-onlyoptions, but given the small number of relevant tests, this seems too complex for now.The
--full-suiteoption currently has no effect, because the only tests parameterized over the set ofFileSystemRunnersinScalarTestConfigare theGitMoveRenameTests, and since these are markedNeedsUpdatesForNonVirtualizedModethey don't run at all. With theExtraCoveragecategory removed, this means the difference between the WindowsRunFunctionalTests.batscript and the MacRunFunctionalTests.shone (where the latter uses--full-suitebut not the former) no longer has an effect, and theFetchStepFailsWhenItCannotRemoveABadPrefetchPack()test will now run, and fail, on Windows CI jobs.We could remove
--full-suitebut instead choose to simply align the MacRunFunctionalTests.shscript with the Windows equivalent (i.e., not passing that option by default), and then adding the option to both platform's scripts in our Azure Pipelines configurations. This way, if the option does begin to have an effect again in the future, it will run on all CI jobs but won't slow down manual tests.We update the
AuthoringTests.mddocumentation to reflect these changes as well as others which have been introduced since this project diverged from VFSForGit, particularly the removal of a number of specialized test programs,classes, and options.
Another cleanup we make is to remove the creation of the
EnableGitStatusCacheToken.datfile on Windows by the functional test program, since the relevant implementation was removed in PR #29.Lastly, we address the skipped-and-failing
FetchStepFailsWhenItCannotRemoveABadPrefetchPack()functionaltest, which was looking for the string
"Unable to delete"in the output from a Scalarrun fetchcommand. That string is no longer output byReportErrorAndExit()as a result of PR #295, which introduced a common progress meter to theruncommand. 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.The test is also Windows-specific because it depends on an open file handle with a
FileShare.Noneattribute blocking attempts by other processes to delete the file, and this only works in C# on the Windows platform. On Unix platforms, the C# core libraries implement file sharing in an advisory manner only, meaning other programs may simply delete the file.We therefore mark this test as
WindowsOnly, since it effectively does not apply to other platforms. Note that the previous category assigned to the test,MacTODO.TestNeedsToLockFile, dates from VFSForGit and has been migrated to just this one test as a result of changes in PR #170. However, the meaning of that category varies, so we clarify the situation by just marking thisWindowsOnly.