[POC] Add Tests for HandleVnodeOperation #3

jeschu1 · 2019-03-19T14:30:00Z

This is obviously an incomplete first pass at trying to test from top level HandleVnodeOperation.

I have several questions that I've put inline.

jeschu1 · 2019-03-19T16:49:31Z

ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm

+    int* kauthError)
+{
+/*
+   MockCalls::RecordFunctionCall(


@pmj can you tell what's wrong with the RecordFunctionCall here? I must be missing something subtle cause it says it doesn't match what's expected.

Once I have this I can validate what we called in the test.

Ouch, this is C++ at its worst. I've put a workaround up here: https://github.com/pmj/VFSForGit/tree/more_testing_handlevnodeop - I'll need to go in and finish off the argument checking implementation to know whether this'll work long term. For now, please use the workaround, I'll worry about the fallout. :-)

jeschu1 · 2019-03-19T16:52:07Z

ProjFS.Mac/PrjFSKext/ProviderMessaging.cpp

@@ -124,6 +124,7 @@ void ProviderMessaging_AbortOutstandingEventsForProvider(VirtualizationRootHandl
    Mutex_Release(s_outstandingMessagesMutex);
 }

+#ifndef KEXT_UNIT_TESTING


Thoughts on this approach? I don't believe we have anything similar yet, and I can see this being the only function we'd want to leave out for now.

There is also the swap approach suggested in this PR. microsoft#732 (review) , but I wasn't fully sure on implementation.

I think the swap approach is going to be the long term solution, but for now this is fine, at least as long as we'r not trying to write tests for ProviderMessaging_TrySendRequestAndWaitForResponse itself. We can eventually also expand MockCalls to generate a function to swap with automatically, so we don't have to have these dummy functions forwarding to MockCalls::RecordFunctionCall everywhere; it'll be super slick to use if we can pull off the gnarly C++ template back-end.

Thoughts on this approach?

@jeschu1 I don't think we need the #ifndef KEXT_UNIT_TESTING here, because it looks like we don't need to compile ProviderMessaging.cpp as part of PrjFSKextTestable at all (I pushed a branch with this change, see wilbaker@6acd456)

I think this approach is OK for now if we find spots that need it, but at this point I'm not sure if the unit tests need to call any of the functions in ProviderMessaging.cpp.

cc: @pmj for his thoughts as well

Related, can anyone remember if there was a need to compile ProviderMessaging.cpp as part of Testable?

I'm thinking this might have just been picked up by mistake (and didn't cause any issues until now).

Related, can anyone remember if there was a need to compile ProviderMessaging.cpp as part of Testable?

I'm thinking this might have just been picked up by mistake (and didn't cause any issues until now).

I think I included it when moving those functions to the new file to ensure that they successfully compiled in the user space/testable build environment, and that I hadn’t for example missed some headers. We can certainly take it back out of the testable build for now, I don’t think any of my WIP PRs will be testing those functions just yet.

ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm

jeschu1 · 2019-03-19T16:54:58Z

ProjFS.Mac/PrjFSKextTests/MockProc.cpp

@@ -22,3 +22,43 @@ proc_t vfs_context_proc(vfs_context_t ctx)
 {
    return NULL;
 }
+


While doing this work, it highlighted 2 more areas to unit test

CurrentProcessWasSpawnedByRegularUser

UseMainForkIfNamedStream

jeschu1 · 2019-03-19T16:58:01Z

ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm

+    // Register provider for the repository path (Simulate a mount)
+    VirtualizationRootResult result = VirtualizationRoot_RegisterProviderForPath(&dummyClient, dummyClientPid, repoPath);
+    XCTAssertEqual(result.error, 0);
+    vnode_put(s_virtualizationRoots[result.root].rootVNode);


@pmj I saw the same lines in your unit test. How will the system clean this up for real scenarios?

This is normally done in ActiveProvider_Disconnect() which last I checked wasn't quite ready to be called from unit tests yet. You could give it a go if you like (and the situation may have since improved).

pmj

This is looking good!

pmj · 2019-03-19T17:16:50Z

ProjFS.Mac/PrjFSKext/ProviderMessaging.cpp

@@ -124,6 +124,7 @@ void ProviderMessaging_AbortOutstandingEventsForProvider(VirtualizationRootHandl
    Mutex_Release(s_outstandingMessagesMutex);
 }

+#ifndef KEXT_UNIT_TESTING


I think the swap approach is going to be the long term solution, but for now this is fine, at least as long as we'r not trying to write tests for ProviderMessaging_TrySendRequestAndWaitForResponse itself. We can eventually also expand MockCalls to generate a function to swap with automatically, so we don't have to have these dummy functions forwarding to MockCalls::RecordFunctionCall everywhere; it'll be super slick to use if we can pull off the gnarly C++ template back-end.

pmj · 2019-03-19T17:19:55Z

ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm

+    int* kauthError)
+{
+/*
+   MockCalls::RecordFunctionCall(


Ouch, this is C++ at its worst. I've put a workaround up here: https://github.com/pmj/VFSForGit/tree/more_testing_handlevnodeop - I'll need to go in and finish off the argument checking implementation to know whether this'll work long term. For now, please use the workaround, I'll worry about the fallout. :-)

ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm

- This will mark the file as D in placeholders.dat and insert into modifiedPaths.dat - This cleans up the existing ModifiedFile event which was also being used as the initial switch from placeholders.dat

jeschu1 · 2019-03-19T18:37:56Z

ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm

+
+    // Ensure a directory is Deleted
+    expectedMessageType[0] = MessageType_KtoU_NotifyDirectoryPreDelete;
+    expectedMessageType[1] = MessageType_KtoU_RecursivelyEnumerateDirectory;


@pmj FYI
If we verify DidCallFunction with args it could come up as too separate functions.

Yep, I fully intend to be able to check call sequences; maybe not in the first iteration though.

jeschu1 · 2019-03-19T18:48:01Z

@wilbaker @ jrbriggs this is still a WIP but I'm super happy about the direction. I was able to reuse a lot of the good work @pmj did in this PR microsoft#908 to test from the top level.

Next steps here are

HandleVnodeOperation deserves it's own class with tests focused on Action Types
@pmj is going to help extend the MockCall library to verify parameters. Lots of template magic here and he's best suited.

…e Access is requested Mac: Add PreConvertToFull when Write Access is requested

Additional Kext Tests

Workaround for kext testing MockCalls::RecordFunctionCall issues Additional Vnode Tests Moved to a new file

jeschu1 force-pushed the more_testing_handlevnodeop branch 3 times, most recently from 2c60594 to cf56c0a Compare March 19, 2019 16:47

jeschu1 commented Mar 19, 2019

View reviewed changes

ProjFS.Mac/PrjFSKextTests/KauthHandlerTests.mm Outdated Show resolved Hide resolved

jeschu1 commented Mar 19, 2019

View reviewed changes

pmj reviewed Mar 19, 2019

View reviewed changes

Mac: Add PreConvertToFull when Write Access is requested

cada100

- This will mark the file as D in placeholders.dat and insert into modifiedPaths.dat - This cleans up the existing ModifiedFile event which was also being used as the initial switch from placeholders.dat

jeschu1 force-pushed the more_testing_handlevnodeop branch from 0cc09f9 to 6948945 Compare March 19, 2019 18:36

jeschu1 commented Mar 19, 2019

View reviewed changes

jeschu1 force-pushed the more_testing branch from bd6bf40 to 158468b Compare March 20, 2019 14:57

jeschu1 added 3 commits March 20, 2019 07:59

Merge pull request microsoft#791; Mac: Add PreConvertToFull when Writ…

10b2041

…e Access is requested Mac: Add PreConvertToFull when Write Access is requested

Merge pull request microsoft#913: Additional Kext Tests

2c3e175

Additional Kext Tests

Add Tests for HandleVnodeOperation

9602320

Workaround for kext testing MockCalls::RecordFunctionCall issues Additional Vnode Tests Moved to a new file

jeschu1 force-pushed the more_testing_handlevnodeop branch from 6948945 to 276cc19 Compare March 21, 2019 13:31

IN PROGRESS

f6cc7c1

jeschu1 force-pushed the more_testing_handlevnodeop branch from 276cc19 to f6cc7c1 Compare March 21, 2019 14:35

pmj and others added 2 commits March 21, 2019 10:36

Mac kext testing: Implementation of mock function argument checking

66066e9

Added Args

ca6cef4

[POC] Add Tests for HandleVnodeOperation #3

Are you sure you want to change the base?

[POC] Add Tests for HandleVnodeOperation #3

Uh oh!

Conversation

jeschu1 commented Mar 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeschu1 Mar 19, 2019 • 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!

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!

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!

pmj 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeschu1 commented Mar 19, 2019

Uh oh!

Uh oh!

jeschu1 Mar 19, 2019 •

edited

Loading