Mac: Support basic projection changes and re-categorize functional tests by wilbaker · Pull Request #220 · microsoft/VFSForGit

wilbaker · 2018-08-29T17:28:42Z

Summary of VFSForGit product changes

Added update\delete placeholder functionality to ProjFS.Mac
Update VFSForGit (for Mac) to add files\folders to the placeholder list as they are created during enumeration
Fixed bugs\issues in MacFileSystemVirtualizer.UpdatePlaceholderIfNeeded

With these changes most projection change scenarios are covered. #219 has been filed for supporting projection changes where new files need to be created by VFSForGit (something not covered by the changes in this PR).

Summary of test changes

Eliminated --mac-only flag and replaced it with an OS check in the functional tests
Switched from a allow-list of tests that should run on Mac to a deny-list of tests that should not run on Mac
Fixed up Windows specific paths
Enabled many more tests on Mac (283 now passing, up from 204)

 Test Run Summary
   Overall result: Warning
   Test Count: 286, Passed: 282, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 4
     Skipped Tests - Ignored: 4, Explicit: 0, Other: 0
     Start time: 2018-08-29 23:15:15Z
     End time: 2018-08-29 23:28:25Z
     Duration: 790.114 seconds

Follow up work items
#219
#223
#231

Fixes #196

sanoursa · 2018-08-29T17:49:30Z

GVFS/GVFS.FunctionalTests/Program.cs

@@ -60,17 +60,20 @@ public static void Main(string[] args)

            if (runner.HasCustomArg("--windows-only"))


Do we still need the --windows-only flag? The platform check below should cover this too, right?

We're still using thethe --windows-only flag to restrict GVFS.FunctionalTests.Windows.exe to running the Windows specific tests.

From RunFunctionalTests.bat:

dotnet %~dp0\..\..\BuildOutput\GVFS.FunctionalTests\bin\x64\%Configuration%\netcoreapp2.1\GVFS.FunctionalTests.dll %2 %3 %4 %5 || goto :endFunctionalTests %~dp0\..\..\BuildOutput\GVFS.FunctionalTests.Windows\bin\x64\%Configuration%\GVFS.FunctionalTests.Windows.exe --windows-only %2 %3 %4 %5 || goto :endFunctionalTests

We could always run all the tests with both exes but it would slow down the tests.

Outside the scope of this PR, but do we need to maintain the ability to run the core tests in the .NET Framework project, if we never run them from that project? Not saying we should necessarily, but it seems like we could simplify this so that the .NET Framework test project contains only the Windows-specific tests. Any reason not to?

Any reason not to?

The only reason I can think of (and maybe it's not a good enough reason to keep it), is that it's an easy way for developers to run all of the functional tests from the IDE (they can simply run the .NET Framework project).

I'm also wondering the reverse, is there any reason to run .NET Core version of the functional tests on Windows? Or should we just run them all with the .NET Framework?

I can't remember I ran the functional tests from the IDE :-). If that's something you do, I'm totally fine with making it easier.

Running all of the Windows tests on .NET Framework may actually make more sense, since the Windows client is still using .NET Framework. The current setup is also working well enough, so this is a someday-maybe clean up as far as I'm concerned.

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/CaseOnlyFolderRenameTests.cs

GVFS/GVFS.Platform.Mac/MacFileSystemVirtualizer.cs

ProjFS.Mac/PrjFSLib.Mac.Managed/VirtualizationInstance.cs

ProjFS.Mac/PrjFSLib/PrjFSLib.cpp

sanoursa · 2018-08-29T18:00:12Z

ProjFS.Mac/PrjFSLib/PrjFSLib.cpp

+        << std::hex << updateFlags << std::dec << ")" << std::endl;
+#endif
+
+    PrjFS_Result result = PrjFS_DeleteFile(relativePath, updateFlags, failureCause);


Is this just a temporary approach? Will we be able to update the existing placeholder in place?

I was not thinking this (delete+add) would be a temporary approach:

ProjFS on Windows internally does a delete+add when updating placeholders

Git does a delete+add when updating the working tree as part of a checkout

We do need to compare content ID, but I was thinking we'd stick with delete+add so that applications see the same events with VFSForGit as they do with Git.

ProjFS.Mac/PrjFSLib/PrjFSLib.h

…data for correlation When matching up data with ETW event there is an EnlistmentId and mountId that it tied to data about the enlistment and the mounted process. Other events are not tied to this data so when an exception happens getting the mount data that corresponds to it is not directly correlated. This adds the EnlistmentId and MountId to the ETW data so that the data can be correlated.

… with that category

wilbaker · 2018-08-30T00:14:50Z

@sanoursa I believe I've addressed\replied to all of your feedback so far. The latest changes are ready for another look.

sanoursa

Looks good!

sanoursa · 2018-08-30T16:41:08Z

GVFS/GVFS.FunctionalTests/Categories.cs

-            public const string M1 = "M1_CloneAndMount";
+            // The FailsOnBuildAgent category is for tests that pass on dev
+            // machines but not on the build agents
+            public const string FailsOnBuildAgent = "FailsOnBuildAgent";


I'm just curious, any theories as to why some of the tests are failing on the build agent?

@nickgra had investigated this a bit. The failures all have to do with non-ASCII file names, and I think Nick mentioned there must be a setting on the build agents that impacts how the file names are displayed.

From what I remember it was something to do with continuation characters being added into the names, @nickgra, do you remember any of the details?

You've nailed it, that's about as far as I got in the investigation. I have some ideas on how we can debug this further, I'll follow up offline.

sanoursa · 2018-08-30T16:42:41Z

GVFS/GVFS.FunctionalTests/Program.cs

@@ -60,17 +60,20 @@ public static void Main(string[] args)

            if (runner.HasCustomArg("--windows-only"))


Outside the scope of this PR, but do we need to maintain the ability to run the core tests in the .NET Framework project, if we never run them from that project? Not saying we should necessarily, but it seems like we could simplify this so that the .NET Framework test project contains only the Windows-specific tests. Any reason not to?

GVFS/GVFS.Platform.Mac/MacFileSystemVirtualizer.cs

ProjFS.Mac/PrjFSLib/PrjFSLib.cpp

…s and re-categorize functional tests Mac: Support basic projection changes and re-categorize functional tests

Mac: Support basic projection changes and re-categorize functional tests

295532d

wilbaker added the domain: user-mode label Aug 29, 2018

wilbaker self-assigned this Aug 29, 2018

wilbaker and others added 2 commits August 29, 2018 10:59

Fix StyleCop errors

2013bd3

Add enlistment id and mount id to data for ETW

d58b8b7

sanoursa reviewed Aug 29, 2018

View reviewed changes

wilbaker and others added 6 commits August 29, 2018 12:19

Fix failing functional test

aaaf057

Create FailsOnBuildAgent category and tag EditFileNeedingUtf8Encoding…

fa6143f

… with that category

Changes for PR Feedback

9b29bf0

Exclude Categories.MacTODO.FailsOnBuildAgent tests on Mac

1d2ba61

Fix fileSystemCallbacks bug in UpdatePlaceholderIfNeeded unit test

b206f3e

wilbaker mentioned this pull request Aug 30, 2018

TEST ONLY: Reduce the number of test runners in the Windows full suite #226

Closed

sanoursa approved these changes Aug 30, 2018

View reviewed changes

More changes for PR feedback, and fix erroneous logging in kext

1732df9

nickgra approved these changes Aug 30, 2018

View reviewed changes

wilbaker added 2 commits August 30, 2018 14:53

Fix warning that breaks Release build

5a0715e

Merge pull request microsoft#220 Mac: Support basic projection change…

69c36fd

…s and re-categorize functional tests Mac: Support basic projection changes and re-categorize functional tests

wilbaker closed this Aug 31, 2018

wilbaker mentioned this pull request Sep 4, 2018

Mac: Support basic projection change scenarios #196

Closed

jrbriggs mentioned this pull request Oct 12, 2018

Merge milestones/M142 to releases/shipped #381

Merged

		@@ -60,17 +60,20 @@ public static void Main(string[] args)

		if (runner.HasCustomArg("--windows-only"))

Conversation

wilbaker commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilbaker Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wilbaker commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanoursa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wilbaker commented Aug 29, 2018 •

edited

Loading

wilbaker Aug 29, 2018 •

edited

Loading

wilbaker commented Aug 30, 2018 •

edited

Loading