Clean up functional test MacTODO categories and Windows-only option#421
Merged
derrickstolee merged 5 commits intomicrosoft:mainfrom Aug 17, 2020
Merged
Clean up functional test MacTODO categories and Windows-only option#421derrickstolee merged 5 commits intomicrosoft:mainfrom
derrickstolee merged 5 commits intomicrosoft:mainfrom
Conversation
When the temporary clear() calls were removed from the functional test category lists in PR microsoft#50 (see commit 7e67496), this following block to re-exclude tests marked MacOnly became redundant, so we can remove it now.
In PR microsoft#216 extensive work was done to remove some of the repository registry support of the Scalar service, and many tests no longer required the MacTODO.NeedsServiceVerb category flag and could be successfully executed on the Mac. One test which appears to have been missed was the SecondCloneSucceedsWithMissingTrees() test in SharedCacheTests.cs; however, it also succeeds on the Mac now and so we can remove this functional test category entirely.
In the VFSForGit project, support for upgrading was added in stages, and at first the Mac platform did not have a full implementation of the local config class and "config" verb, so the relevant tests were marked to be excluded on the Mac. Subsequently, full support for the "config" verb was added, but the tests were not enabled on the Mac. So this test category is no longer meaningful, and the tests would succeed on the Mac if several other changes were made (i.e., the NeedsUpdatesForNonVirtualizedMode category was also removed from these tests, and the functional test suite is run as root). While there may not be an intention to ever run these tests, at a minimum we can simplify the test configuration by removing this category. See microsoft/VFSForGit@77fa996 and microsoft/VFSForGit@7511528.
The --windows-only functional test command-line option causes only tests in the WindowsOnly category to be run; since this makes sense on Windows exclusively, we move the parsing of this command-line argument into a Windows-only block, so it will be ignored on other platforms.
derrickstolee
approved these changes
Aug 17, 2020
| // Shutdown the watchman server now that the tests are complete. | ||
| // Allows deleting the unwatched directories. | ||
| ProcessHelper.Run("watchman", "shudown-server"); | ||
| ProcessHelper.Run("watchman", "shutdown-server"); |
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.
Prior to starting work to support runs of the functional test suite on Linux, we can simplify some of the tests marked
MacTODOas no longer needing that designation and also simplify some of the Windows-related configuration and options.On the Windows side of thing, we can remove the second invocation of
excludeCategories.Add(Categories.MacOnly)because it is always performed on Windows in theif/elseblock just above it.We also only parse the
--windows-onlyfunctional test command-line option when running on Windows, as it has no effect on macOS since theWindowsOnlytests are always excluded there.On the macOS side of things, there is one test still marked
MacTODO.NeedsServiceVerbwhich appears to have been missed in PR #216, where that category was removed from many other tests. ThisSecondCloneSucceedsWithMissingTreestest succeeds on macOS as written, so it seems OK to remove theMacTODOflag on it.The other macOS change we make is to remove the
MacTODO.NeedsScalarConfigcategory, which currently applies to all the tests inMultiEnlistmentTests/ConfigVerbTests.cs. These tests are also marked withNeedsUpdatesForNonVirtualizedModeand therefore will continue to not run even after this change. (However, limited testing by disabling theconfigverb's "elevated privileges" checks and removingNeedsUpdatesForNonVirtualizedModefrom theConfigVerbTestsindicates that, in fact, these tests would all succeed on macOS/Linux with some further adjustments. But for now we just remove the "extra"MacTODOexclusion on these tests.)These two changes allow us to remove two of the three
MacTODOcategories entirely.We also do minor housekeeping by fixing one typo in the Watchman
shutdown-servercommand.